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!: Improve sizing behaviour of Dialog #1329

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

VincentSmedinga
Copy link
Contributor

Aligns the (maximum) block and inline sizes of a Dialog with our spacing system.

A Dialog now gets

  • as large as necessary to fit its content,
  • with a maximum that leaves a medium grid space of margin at all sides,
  • and a horizontal maximum of 48 rems.

The inner layout has been improved as well: making the <dialog> a flex container automatically makes its child <form> size optimally. Setting heights for the grid rows of the form no longer seems necessary.

It is a breaking change because the dialog-form-max-block-size token gets removed.

Copy link
Contributor

@RubenSibon RubenSibon left a comment

Choose a reason for hiding this comment

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

I don't have much to add to Aram's comment, but I did notice that this example of how to open a dialog doesn't look right.

Schermafbeelding 2024-07-10 140045

@VincentSmedinga
Copy link
Contributor Author

VincentSmedinga commented Jul 10, 2024

I don't have much to add to Aram's comment, but I did notice that this example of how to open a dialog doesn't look right.

Fixed – the new display: flex overrode the UA style of display: none for a closed dialog…

@github-actions github-actions bot temporarily deployed to demo-DES-785-dialog-margin July 10, 2024 13:54 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-785-dialog-margin July 11, 2024 14:18 Destroyed
There is no functional difference because `position: fixed` positions the dialog relative to the viewport. Bonus: they are direction-agnostic.
@github-actions github-actions bot temporarily deployed to demo-DES-785-dialog-margin July 12, 2024 10:10 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-785-dialog-margin July 12, 2024 10:26 Destroyed
@VincentSmedinga VincentSmedinga merged commit a54d239 into develop Jul 12, 2024
6 checks passed
@VincentSmedinga VincentSmedinga deleted the feature/DES-785-dialog-margin branch July 12, 2024 10:37
@github-actions github-actions bot mentioned this pull request Jul 25, 2024
@@ -21,18 +27,14 @@
.ams-dialog__form {
display: grid;
gap: var(--ams-dialog-form-gap);
grid-template-rows: auto 1fr auto;
max-block-size: var(--ams-dialog-form-max-block-size);
Copy link
Contributor

Choose a reason for hiding this comment

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

This broke the scrolling inside the Dialog (while keeping the action buttons in view).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants