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: Add a nicer summary view for trip planner output #2204

Merged
merged 18 commits into from
Nov 18, 2024

Conversation

joshlarson
Copy link
Contributor

@joshlarson joshlarson commented Oct 23, 2024

Before

Screenshot 2024-10-23 at 2 19 41 PM

After

Screenshot 2024-10-23 at 2 20 25 PM


Parent Ticket: Trip Planner Preview | Output summary view
Subtask Ticket: Add mode icons (not worrying about the smushed icons for now) to the summary view


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?

Assorted Implementation Notes

  • This does not fully solve the parent ticket - there are some elements missing, and the icons won't always be right. I'm treating those as follow-up tasks.
  • Smushed icons are a follow-up ticket. Showing icons side-by-side will be part of this ticket, but also a follow-up PR. This will require changing the ItineraryGroups algorithm to group the legs within itineraries as well as grouping similar itineraries themselves.
  • I wasn't able to use the Metro badges for this because they rendered incorrectly. Happy to fix that now if it's easy, but otherwise I don't want to block the functionality here on fixing the library, especially since the icons I had already built do do the job correctly.
    Screenshot 2024-10-23 at 5 46 50 PM

@joshlarson joshlarson marked this pull request as ready for review October 23, 2024 21:53
@joshlarson joshlarson requested a review from a team as a code owner October 23, 2024 21:53
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.

Feel free to do some refactoring as it makes sense for you! Left a few comments.

lib/dotcom_web/components/mode_icons.ex Outdated Show resolved Hide resolved
lib/dotcom_web/components/mode_icons.ex Outdated Show resolved Hide resolved
lib/dotcom_web/components/svg.ex Outdated Show resolved Hide resolved
lib/dotcom_web/components/trip_planner/itinerary_group.ex Outdated Show resolved Hide resolved
@anthonyshull
Copy link
Contributor

I don't think any of those badge or icon components are necessary.

@anthonyshull
Copy link
Contributor

Can definitely use metro now to put thos SVGs into a directory and turn them into metro icons. Then put that icon into a circle badge.

@joshlarson joshlarson added the dev-blue Deploy to dev-blue label Oct 29, 2024
@joshlarson joshlarson force-pushed the jdl/trip-planner-summary-view branch from 514b6e3 to 8f83729 Compare October 30, 2024 18:29
assets/tailwind.config.js Outdated Show resolved Hide resolved
@joshlarson joshlarson removed the dev-blue Deploy to dev-blue label Oct 31, 2024
@joshlarson joshlarson force-pushed the jdl/trip-planner-summary-view branch 2 times, most recently from 2532d67 to dba9d1c Compare November 6, 2024 20:40
@thecristen thecristen force-pushed the jdl/trip-planner-summary-view branch from 467b27b to 8e71c85 Compare November 7, 2024 22:31
end

# Group of commuter rail routes are summarized to one symbol.
defp leg_icon(%{routes: [%Routes.Route{type: 2} | _]} = assigns) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non-blocking, partly because I think my ideal outcome here might be a fairly large restructuring of how routes and modes are represented in dotcom (which is definitely too much scope for this PR), but I feel like there are a lot of places where I have to memorize what type: 2/type: 4/etc mean in order to understand our code.

|> assign(
:grouped_classes,
if(slashed?,
do: "[&:not(:first-child)]:rounded-l-none [&:not(:last-child)]:rounded-r-none !px-3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-11-15 at 5 06 22 PM Screenshot 2024-11-15 at 5 15 06 PM

Something is going wonky with respect to the spacing. It looks like for whatever reason, the wrapping div thinks that each bus pill is a bit larger than it is (hence the fact that the weird empty space to the right is bigger when there are three routes versus when there are two).

What's especially weird is that it looks like this is only happening in Firefox. It looks great in Chrome.

@thecristen thecristen merged commit a690232 into main Nov 18, 2024
26 of 27 checks passed
@thecristen thecristen deleted the jdl/trip-planner-summary-view branch November 18, 2024 21:11
@github-actions github-actions bot removed the dev-blue Deploy to dev-blue label Dec 27, 2024
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.

3 participants