feat!: Remove unnecessary Dialog wrapper (#1591)

Amsterdam · Sep 18, 2024 · 450b0c3 · 450b0c3
1 parent 4fb6e53
commit 450b0c3
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 37 deletions.
diff --git a/packages/css/src/components/dialog/dialog.scss b/packages/css/src/components/dialog/dialog.scss
@@ -4,36 +4,33 @@
  */
 
 @mixin reset-dialog {
+  box-sizing: border-box;
   inset-block: 0;
-  padding-block: 0;
-  padding-inline: 0;
 }
 
-.ams-dialog {
+/* A dialog must have `display: none` until the `open` attribute is set,
+so do not apply these styles without an `open` attribute. */
+.ams-dialog:not(dialog:not([open])) {
   background-color: var(--ams-dialog-background-color);
   border: var(--ams-dialog-border);
+  display: flex; // Using flex here, because grid works strangely in Safari
+  flex-direction: column;
+  gap: var(--ams-dialog-gap);
   inline-size: var(--ams-dialog-inline-size);
+  max-block-size: var(--ams-dialog-max-block-size);
   max-inline-size: var(--ams-dialog-max-inline-size);
+  padding-block: var(--ams-dialog-padding-block);
+  padding-inline: var(--ams-dialog-padding-inline);
 
   @include reset-dialog;
 
-  /* no token because dialog does not inherit from any element */
+  /* No token because dialog does not inherit from any element in FireFox and Safari */
   &::backdrop {
     background: #0006;
   }
 }
 
-.ams-dialog__wrapper {
-  display: grid;
-  gap: var(--ams-dialog-wrapper-gap);
-  grid-template-rows: auto 1fr auto;
-  max-block-size: var(--ams-dialog-wrapper-max-block-size);
-  padding-block: var(--ams-dialog-wrapper-padding-block);
-  padding-inline: var(--ams-dialog-wrapper-padding-inline);
-}
-
-.ams-dialog__content {
-  max-block-size: 100%; /* Safari */
+.ams-dialog__body {
   overflow-y: auto;
   overscroll-behavior-y: contain;
 }

diff --git a/packages/react/src/Dialog/Dialog.test.tsx b/packages/react/src/Dialog/Dialog.test.tsx
@@ -26,9 +26,7 @@ describe('Dialog', () => {
 
     const component = screen.getByRole('dialog', { hidden: true })
 
-    expect(component).toHaveClass('extra')
-
-    expect(component).toHaveClass('ams-dialog')
+    expect(component).toHaveClass('ams-dialog extra')
   })
 
   it('supports ForwardRef in React', () => {
@@ -67,9 +65,15 @@ describe('Dialog', () => {
   })
 
   it('renders footer when provided', () => {
-    const { getByText } = render(<Dialog heading="Test heading" footer={<button>Click Me</button>} />)
+    render(<Dialog heading="Test heading" footer={<button>Click Me</button>} open />)
+
+    const footer = screen.getByRole('contentinfo')
+    const button = screen.getByRole('button', {
+      name: 'Click Me',
+    })
 
-    expect(getByText('Click Me')).toBeInTheDocument()
+    expect(footer).toBeInTheDocument()
+    expect(button).toBeInTheDocument()
   })
 
   it('does not render footer when not provided', () => {

diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx
@@ -10,10 +10,10 @@ import { Heading } from '../Heading'
 import { IconButton } from '../IconButton'
 
 export type DialogProps = {
-  /** The button(s) in the footer. Start with a primary button. */
-  footer?: ReactNode
   /** The label for the button that dismisses the Dialog. */
   closeButtonLabel?: string
+  /** The button(s) in the footer. Start with a primary button. */
+  footer?: ReactNode
   /** The text for the Heading. */
   heading: string
 } & PropsWithChildren<DialogHTMLAttributes<HTMLDialogElement>>
@@ -23,18 +23,16 @@ const openDialog = (id: string) => (document.querySelector(id) as HTMLDialogElem
 
 const DialogRoot = forwardRef(
   (
-    { footer, children, className, closeButtonLabel = 'Sluiten', heading, ...restProps }: DialogProps,
+    { children, className, closeButtonLabel = 'Sluiten', footer, heading, ...restProps }: DialogProps,
     ref: ForwardedRef<HTMLDialogElement>,
   ) => (
     <dialog {...restProps} ref={ref} className={clsx('ams-dialog', className)}>
-      <div className="ams-dialog__wrapper">
-        <header className="ams-dialog__header">
-          <Heading size="level-4">{heading}</Heading>
-          <IconButton label={closeButtonLabel} onClick={closeDialog} size="level-4" type="button" />
-        </header>
-        <div className="ams-dialog__content">{children}</div>
-        {footer && <footer className="ams-dialog__footer">{footer}</footer>}
-      </div>
+      <header className="ams-dialog__header">
+        <Heading size="level-4">{heading}</Heading>
+        <IconButton label={closeButtonLabel} onClick={closeDialog} size="level-4" type="button" />
+      </header>
+      <div className="ams-dialog__body">{children}</div>
+      {footer && <footer className="ams-dialog__footer">{footer}</footer>}
     </dialog>
   ),
 )

diff --git a/proprietary/tokens/src/components/ams/dialog.tokens.json b/proprietary/tokens/src/components/ams/dialog.tokens.json
@@ -3,14 +3,12 @@
     "dialog": {
       "background-color": { "value": "{ams.color.primary-white}" },
       "border": { "value": "0" },
+      "gap": { "value": "{ams.space.md}" },
       "inline-size": { "value": "calc(100% - 2 * {ams.space.grid.md})" },
+      "max-block-size": { "value": "calc(100dvh - 2 * {ams.space.grid.md})" },
       "max-inline-size": { "value": "48rem" },
-      "wrapper": {
-        "gap": { "value": "{ams.space.md}" },
-        "max-block-size": { "value": "calc(100dvh - 4 * {ams.space.grid.md})" },
-        "padding-block": { "value": "{ams.space.grid.md}" },
-        "padding-inline": { "value": "{ams.space.grid.lg}" }
-      },
+      "padding-block": { "value": "{ams.space.grid.md}" },
+      "padding-inline": { "value": "{ams.space.grid.lg}" },
       "header": {
         "gap": { "value": "{ams.space.md}" }
       },