Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ShuttleHeadsigns): Merge the replacement shuttle bus cards with … #1769

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

kotva006
Copy link
Contributor

…the commuter rail cards

Summary of changes

Working on display tests, wanted to get some eyes on this

Hide Shuttle Cards from displaying on the new Subway stop page
Merge Shuttle times and headsign into the Commuter Rail Cards on the new Commuter Rail stop page.

Asana Ticket: Shuttle Service is Shown in Separate Departure Cards


General checks

  • Are the changes organized into self-contained commits with descriptive and well-formatted commit messages? This is a good practice that can facilitate easier reviews.
  • Testing. Do the changes include relevant passing updates to tests? This includes updating screenshots. Preferably tests are run locally to verify that there are no test failures created by these changes, before opening a PR.
  • Tech debt. Have you checked for tech debt you can address in the area you're working in? This can be a good time to address small issues, or create Asana tickets for larger issues.

New UI, or substantial UI changes

  • Cross-browser compatibility is less of an issue now that we're no longer supporting IE, but changes still need to work as expected in Safari, Chrome, and Firefox.
  • Are interactive elements accessible? This includes at minimum having relevant keyboard interactions and visible focus, but can also include verification with screen reader testing.
  • Other accessibility checks such as sufficient color constrast, or whether the layout holds up at 200% zoom level.

New endpoints, or non-trivial changes to current endpoints

  • Have we load-tested any new pages or internal API endpoints that will receive significant traffic? See load testing docs
  • If this change involves routes, does it work correctly with pertinent "unusual" routes such as the combined Green Line, Silver Line, Foxboro commuter rail, and single-direction bus routes like the 170?

@kotva006 kotva006 requested a review from a team as a code owner October 16, 2023 19:47
@kotva006 kotva006 requested review from i0sea and thecristen October 16, 2023 19:47
Copy link
Collaborator

@thecristen thecristen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! This review is incomplete, so these comments are just for the back-end piece - will circle back soon.

I think the overall approach is sound, though I had some comments.

Another (non-blocking) question I had - I notice you're doing a lot of manipulation of the grouped route patterns in the frontend, based on the newly obtained route line information - which is fine, but might it be simpler to modify those grouped route patterns before they hit the frontend, when they're pulled together and grouped in the controller?

apps/routes/lib/route.ex Outdated Show resolved Hide resolved
apps/routes/lib/parser.ex Outdated Show resolved Hide resolved
apps/route_patterns/lib/repo.ex Outdated Show resolved Hide resolved
Comment on lines -51 to -56
jest.mock("react-router-dom", () => ({
__esModule: true,
useLoaderData: () => {
return TEST_LOADER_VALUE;
}
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: This was redundant because the data is already passed into the render function inside the helper function renderWithRouter

@kotva006
Copy link
Contributor Author

Thanks!! This review is incomplete, so these comments are just for the back-end piece - will circle back soon.

I think the overall approach is sound, though I had some comments.

Another (non-blocking) question I had - I notice you're doing a lot of manipulation of the grouped route patterns in the frontend, based on the newly obtained route line information - which is fine, but might it be simpler to modify those grouped route patterns before they hit the frontend, when they're pulled together and grouped in the controller?

I ended up putting the fix on the front end because I viewed this issue as a display problem. I was trying to avoid curating the data returned by the route pattern endpoint to fit this one use case. I thought there may be other times where we want these route patterns separate. If this needs to be changed everywhere route patterns are consumed, then it would makes sense to change it on the backend.

@kotva006 kotva006 requested a review from thecristen October 19, 2023 02:12
@thecristen thecristen self-assigned this Oct 19, 2023
Copy link
Collaborator

@thecristen thecristen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes and feedback! It's working as expected. I updated a couple Elixir tests and it passes for me locally - will 🚢

@thecristen thecristen merged commit 1736685 into master Oct 19, 2023
@thecristen thecristen deleted the rk/shuttle-cards branch October 19, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants