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.

Sign up for GitHub

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

Jump to bottom

feat: Update detour list page and table styles for mobile #2774

Merged
merged 1 commit into from
Sep 9, 2024
Merged

feat: Update detour list page and table styles for mobile #2774

merged 1 commit into from
Sep 9, 2024

Conversation

joshlarson
Copy link
Contributor

@joshlarson joshlarson commented Sep 5, 2024 •
edited
Loading

@joshlarson joshlarson requested a review from a team as a code owner September 5, 2024 20:54
joshlarson
joshlarson commented Sep 5, 2024
View reviewed changes
assets/css/utilities/_hideable.scss Outdated
@@ -11,7 +11,7 @@
}

.u-hide-for-mobile {
@media screen and (max-width: map-get($breakpoints, "max-mobile-landscape-tablet-portrait-width")) {
@include media-breakpoint-down(md) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbta/skate-developers want to surface this change. It's not a huge change, but this changes the "hide for mobile" breakpoint from 800px to 768px.

I did this because media-breakpoint-... uses bootstrap's breakpoints, and I wanted to bias towards bootstrap's breakpoints rather than our own _skate_ui ones, especially since we want to stop "caring about what's in _skate_ui", per this Slack Convo.

If you'd all prefer, I can split it out into a separate PR. Or ditch this and find a different way to keep our breakpoints consistent!

Copy link
Member

Choose a reason for hiding this comment

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

Btw I found the variable we need to update if we do want to make bootstrap use our breakpoints as a temporary measure https://github.com/twbs/bootstrap/blob/main/scss/_variables.scss#L483-L492

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 Amazing, thank you!

What do you think of this?

And at this point, should the _variable_overrides change and this one be split to a separate PR, do you think?

Copy link
Member

Choose a reason for hiding this comment

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

And at this point, should the _variable_overrides change and this one be split to a separate PR, do you think?

I'm good either way 👍🏻

What do you think of this?

Looks good, I don't think you need !default? Did you try it without that? AFAIK bootstrap has that in their config so that we can override it easily.

Maybe it also should document why these values are set like this, because right now we're doing this to be compatible with the old breakpoints "system", but I don't know what design will want to do with breakpoints in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment

I think I do want to split this out into two PR's.

@joshlarson joshlarson enabled auto-merge (squash) September 5, 2024 22:10
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 5, 2024

Coverage of commit 76c6374

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson force-pushed the jdl/feat/format-detour-list-table-on-mobile branch from 76c6374 to b4ba629 Compare September 9, 2024 15:10
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit b4ba629

Summary coverage rate:
  lines......: 93.0% (3298 of 3547 lines)
  functions..: 72.4% (1359 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson mentioned this pull request Sep 9, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit c32912c

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit c32912c

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson disabled auto-merge September 9, 2024 15:46
@joshlarson joshlarson marked this pull request as draft September 9, 2024 15:48
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit 6ed52d0

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson force-pushed the jdl/feat/format-detour-list-table-on-mobile branch 2 times, most recently from 7fca2cf to 8efa5ed Compare September 9, 2024 15:52
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit 7fca2cf

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@joshlarson joshlarson changed the base branch from main to jdl/refactor/update-and-use-bootstrap-breakpoints September 9, 2024 15:54
@joshlarson joshlarson marked this pull request as ready for review September 9, 2024 15:55
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit 8efa5ed

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit 8efa5ed

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Base automatically changed from jdl/refactor/update-and-use-bootstrap-breakpoints to main September 9, 2024 15:58
@joshlarson joshlarson force-pushed the jdl/feat/format-detour-list-table-on-mobile branch from 8efa5ed to b30b0d0 Compare September 9, 2024 16:01
@joshlarson joshlarson enabled auto-merge (squash) September 9, 2024 16:02
@joshlarson joshlarson merged commit 7a28a44 into main Sep 9, 2024
9 checks passed
@joshlarson joshlarson deleted the jdl/feat/format-detour-list-table-on-mobile branch September 9, 2024 16:06
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 9, 2024

Coverage of commit b30b0d0

Summary coverage rate:
  lines......: 93.0% (3299 of 3547 lines)
  functions..: 72.5% (1360 of 1876 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@firestack firestack firestack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joshlarson @firestack