Make Dialog footer scrollable on very short viewports #5629
Conversation
🦋 Changeset detectedLatest commit: 40ea8f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
the
integration-tests: recommended
size-limit report 📦
|
hectahertz
temporarily deployed
to
github-pages
GitHub Actions
Inactive
hectahertz
temporarily deployed
to
github-pages
GitHub Actions
Inactive
hectahertz
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| flex-direction: row-reverse; | ||
| flex-wrap: wrap-reverse; |
There was a problem hiding this comment.
I noticed the buttons were reversed, and focus seems to be too. Wasn't sure if this was expected or not 🤔
There was a problem hiding this comment.
They shouldn't be! This is compensated for in the JS code. Let's review this together
There was a problem hiding this comment.
I think the buttons are only reversed on the ConfirmationDialog. If we reverse the array for the footer buttons in that component then I think the issue should be fixed 🤔
| gap: ${get('space.2')}; | ||
| z-index: 1; | ||
| flex-shrink: 0; | ||
|
|
||
| @media (max-height: 256px) { |
There was a problem hiding this comment.
I'm wondering if we might want to increase this a bit, maybe by 50-100px? I noticed that on some screen sizes, the body isn't visible. I think increasing it a bit would fix it.
The only potential issue is if a consumer adds even more buttons, which could mean that even an increase of the max-height wouldn't help. I don't think this is an issue the component should solve, as we wouldn't want consumers adding dozens of buttons in a footer 😅
| flex-direction: row-reverse; | ||
| flex-wrap: wrap-reverse; | ||
| gap: ${get('space.2')}; | ||
| z-index: 1; | ||
| flex-shrink: 0; | ||
|
|
||
| @media (max-height: 256px) { | ||
| flex-wrap: nowrap; | ||
| overflow-x: scroll; | ||
| } |
There was a problem hiding this comment.
| flex-direction: row-reverse; | |
| flex-wrap: wrap-reverse; | |
| gap: ${get('space.2')}; | |
| z-index: 1; | |
| flex-shrink: 0; | |
| @media (max-height: 256px) { | |
| flex-wrap: nowrap; | |
| overflow-x: scroll; | |
| } | |
| flex-flow: wrap; | |
| justify-content: flex-end; | |
| gap: ${get('space.2')}; | |
| z-index: 1; | |
| flex-shrink: 0; | |
| @media (max-height: 356px) { | |
| flex-wrap: nowrap; | |
| overflow-x: scroll; | |
| flex-direction: row; | |
| justify-content: unset; | |
| } |
Curious what you think about this change? I think this also means we don't have to utilize reverse() on the footer buttons.
Part of the work to improve reflow of the component. This helps to make all the footer buttons accessible on small screens and gives some extra breathing room to the body.
Changelog
Changed
Dialog footer becomes scrollable on very short viewports.
Before
Screen.Recording.2025-01-30.at.20.46.28.mov
After
Screen.Recording.2025-01-30.at.20.46.08.mov
Rollout strategy
Testing & Reviewing
The CSS changes seem a bit esoteric, but are due to
overflow: scrollnot being compatible withjustify-content: flex-end.flex-direction: row-reverse; flex-wrap: wrap-reverse;plus reversing the items is visually equivalent but compatible. The focus order keeps being the same.Merge checklist