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: Suppress "Add detour" dropdown on mobile #2716

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

joshlarson
Copy link
Contributor

@joshlarson joshlarson marked this pull request as ready for review July 31, 2024 15:47
@joshlarson joshlarson requested a review from a team as a code owner July 31, 2024 15:47
Copy link

Coverage of commit 8ccebff

Summary coverage rate:
  lines......: 93.0% (3318 of 3566 lines)
  functions..: 72.5% (1356 of 1870 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson enabled auto-merge (squash) July 31, 2024 16:03
@@ -213,6 +214,11 @@ const RouteLadder = ({

const displayCrowding = someVehicleHasCrowding(vehiclesAndGhosts, route.id)

const screenSize = useScreenSize()
Copy link
Member

Choose a reason for hiding this comment

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

can we do this within CSS instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could - I think that would make it a bit more difficult to test, but perhaps that's okay?

Copy link
Member

@firestack firestack Aug 1, 2024

Choose a reason for hiding this comment

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

Yeah I'm not really sure we'd be able to test it until we have CSS in Jest tests, but that's fine with me. We could assert a certain class is on it, I think we just need to change it to d-none d-sm-block(replace block with whatever it's supposed to be), or something like that https://getbootstrap.com/docs/5.3/utilities/display/ (see the Hidden only on xs text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@firestack I updated to match ☝️, but I'm not sure that I love the lack of testability. I'll let it sit with me over the evening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@firestack I've let it sit with me, and while I don't love relying on snapshot tests, they will solve this issue.

Ready for re-review!

@joshlarson joshlarson disabled auto-merge August 1, 2024 21:26
@joshlarson joshlarson requested a review from firestack August 1, 2024 21:27
Copy link

github-actions bot commented Aug 1, 2024

Coverage of commit 31c7a56

Summary coverage rate:
  lines......: 93.1% (3319 of 3566 lines)
  functions..: 72.6% (1357 of 1870 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson enabled auto-merge (squash) August 2, 2024 18:13
@joshlarson joshlarson merged commit 0f120e8 into main Aug 5, 2024
15 checks passed
@joshlarson joshlarson deleted the jdl/feat/suppress-add-detour-dropdown-on-mobile branch August 5, 2024 14:05
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