fix(styles): introduce modal max-height#2249
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Pull request overview
This PR addresses #2125 by constraining dialog/modal height so long modal content can be scrolled instead of pushing footer controls below the fold.
Changes:
- Add a max-height constraint to
.Dialog__innerandoverflow-y: autoto.Dialog__content. - Add Modal visual-regression coverage via new Playwright screenshot tests and baseline images.
- Add an accessibility (axe) regression test case for long modal content.
Reviewed changes
Copilot reviewed 3 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/styles/dialog.css | Adds max-height + scroll behavior for dialog content. |
| packages/react/src/components/Modal/screenshots.e2e.tsx | New Modal screenshot coverage for small/large content across viewports and themes. |
| packages/react/src/components/Modal/index.test.tsx | Adds axe tests intended to cover long-content scenarios. |
| e2e/screenshots/modal-small-content.png | New baseline screenshot. |
| e2e/screenshots/dark--modal-small-content.png | New baseline screenshot (dark theme). |
| e2e/screenshots/modal-large-content-large-viewport.png | New baseline screenshot (large viewport). |
| e2e/screenshots/modal-large-content-small-viewport.png | New baseline screenshot (small viewport). |
| e2e/screenshots/dark--modal-large-content-small-viewport.png | New baseline screenshot (dark + small viewport). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Zidious
left a comment
Zidious
left a comment
There was a problem hiding this comment.
This is looking great! Left some minor comments, also the 🤖 has reasonable comments.
Small nit: we'd want to scope your title to something like fix(react): ... so we can have the changelog generation scope the fix to the correct CHANGELOG file (see conventional commits docs).
gupta-tanvi26
changed the title
maksym-shynkarenko
left a comment
maksym-shynkarenko
left a comment
There was a problem hiding this comment.
LGTM, but I'm not really familiar with cauldron. Leave the final review on @anastasialanz
gupta-tanvi26
dismissed stale reviews from chornonoh-vova and Bracciata
via
be1bd51
gupta-tanvi26
dismissed stale reviews from Zidious, maksym-shynkarenko, and shawnsharpDQ
via
be1bd51
gupta-tanvi26
dismissed stale reviews from Bracciata and chornonoh-vova
via
d5eab76
|
This change introduced unintended changes in one of the modals in extension because of the new max-height. |
2249.mov
This PR fixes situations when the length of the modal content exceeds the viewport. In case of modal content overflowing on large viewports, we want the modal content to be scrollable while the footer buttons stay fixed at the bottom of the screen, above the fold. On small viewports, the footer buttons can go below the fold of the screen and we should be able to scroll through the entire modal and not just the modal content.
The following are the breakpoints we follow -
@Viewport:320px (20rem)
@Viewport:768px (48rem)
@Viewport:1280px (64rem)
We want anything above 320px to have scrollable modal content.
Closes #2125