fix: Avoid collapsing green line and Mattapan line rows unless needed #2415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Base automatically changed from
jdl/subway-status/collapsing-logic-tests
to
main
March 5, 2025 16:57
joshlarson
force-pushed
the
jdl/system-status/prioritized-collapsing
branch
from
deab2ed to
abdb8f1
Compare
joshlarson
commented
Mar 5, 2025
thecristen
requested changes
Mar 5, 2025
There was a problem hiding this comment.
The before and after is soooo good!
I almost feel like there might be a meta-programming-ish thing one could implement around the three collapse functions, since they all work so similarly. But I didn't leave a blocking comment on it, since this implementation is probably clearer anyway, despite the repetition.
Requesting changes for adjusting the "active now" part of your test code.
test/dotcom_web/components/system_status/subway_status_test.exs
Outdated
Show resolved
Hide resolved
joshlarson
force-pushed
the
jdl/system-status/prioritized-collapsing
branch
from
c18768d to
dd36096
Compare
joshlarson
commented
Mar 5, 2025
| @@ -131,7 +131,7 @@ defmodule DotcomWeb.Components.SystemStatus.SubwayStatus do | |||
| @route_ids | |||
| |> Enum.map(&{&1, subway_status |> Map.get(&1)}) | |||
| |> Enum.flat_map(&rows_for_route/1) | |||
| |> maybe_collapse_rows() | |||
| |> collapse_rows_if_needed() | |||
There was a problem hiding this comment.
I guess I could name this back to maybe_collapse_rows(). I can't remember now why I changed it.
…hod to use Also simplify names
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a few edge cases in the homepage subway status component where it was unnecessarily collapsing green line rows together, or collapsing Mattapan alerts into Red line (which we decided it should never do).
Two Green line rows when other data would cause a collapse (Green line shouldn't be collapsed, but other rows should be)
Before
After
Green line and Mattapan both have extra rows (Green should be collapsed / Mattapan should remain visible)
Before
After
Asana Tickets
Blocked by: