refactor(dialog): add more header flexibility #4000

marissahuysentruyt · 2025-06-26T17:59:52Z

Description

This work adds a container query to the .spectrum-Dialog element. The container query should allow fullscreen/fullscreenTakeover dialogs to reflow the content grid sooner. The ability to reflow sooner should be useful when implementations and users elect to add a component to the dialog's header area (i.e. the steplist in the fullscreen dialog example), allowing the dialog heading to fill up more space instead of wrapping so much, even on large-ish screens.

Because of the container query, however, extra containerStyles and wrapperStyles were added to the test canvases to make sure the entire dialog is captured, instead of just the minimum inline sizes.

Jira/Specs

CSS-1092

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Jira/Specs

CSS-1092

Validation steps

Pull down the branch or visit the deploy preview. (@rise-erpelding)
Navigate to the dialog docs page. Verify nothing on that page has changed (you can compare to the spectrum-two preview).
Visit the full screen dialog story page.
In your browser, change the viewport size and verify that the fullscreen grid changes earlier for smaller screens, and allows the heading and extra header component to wrap as before.
Repeat this process for the full screen takeover dialog story.
If you remove the .spectrum-Dialog-headerContentWrapper element and its contents in your browser inspector, the dialog's heading fills all of its space.
For both full screen dialog variants, I'd love to get extra validation to make sure the original 700px media query isn't interfering with the 864px container query.

Additional validation

The dialog tests return no VRT diffs

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

🚫 Before (820px)

✅ After (also 820px)

To-do list

I have read the contribution guidelines.
✨ This pull request is ready to merge. ✨

changeset-bot · 2025-06-26T17:59:56Z

🦋 Changeset detected

Latest commit: 9a94096

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@spectrum-css/dialog	Minor
@spectrum-css/bundle	Patch
@spectrum-css/preview	Patch

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

components/dialog/index.css

github-actions · 2025-06-26T18:05:22Z

File metrics

Summary

Total size: 1.43 MB^*

Package	Size	Minified	Gzipped
dialog	16.64 KB	15.91 KB	2.30 KB

dialog

Filename	Head	Minified	Gzipped	Compared to base
index.css	16.64 KB	15.91 KB	2.30 KB	🔴 ⬆ 0.28 KB
metadata.json	6.58 KB	-	-	-

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2025-06-26T18:06:01Z

📚 Branch preview

PR #4000 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-4000/index.html.

components/dialog/index.css

marissahuysentruyt · 2025-07-01T13:07:06Z

components/dialog/stories/dialog.test.js

+		"width": "100%", // forces the data-inner-container to be full width so the container query doesn't break.
+	},
+	containerStyles: {
+		"width": "100%", // forces the data-outer-container to be full width so the container query doesn't break.


Without the width: 100%, the dialogs were defaulting to their minimum widths because of the container query. This forces these test containers to be the full width, so the fullscreen dialogs can appropriately be their full widths.

Oh noes! I believe it's the first one (the inner-container) that's creating a horizontal scrollbar at the bottom in the default story (but not the test):

I did play with it for a bit and something like -webkit-fill-available does work rather than 100%, but doesn't work in Firefox.

We could also try using isChromatic for this? I don't know if it's cool to use that for components. But it'd look something like

import isChromatic from "chromatic/isChromatic"; // all the other stuff... wrapperStyles: isChromatic ? { "background-color": "var(--spectrum-gray-50)", "width": "100%", // forces the data-inner-container to be full width so the container query doesn't break. } : {}, // ...all the rest of the stuff

There might be a better solution than that also!

Ok and one more thing on this: would we want to refactor to use inline-size instead of width? I know using logical properties for the storybook wrapper styles is a little less critical than in the styles we ship, so I definitely don't consider that blocking at all.

rise-erpelding

I left only a few comments, but they're lengthy! The container query works great as far as I can see, just left a comment about the margin-block-end on the heading.

I also noted a small issue with the horizontal scrolling in Storybook, otherwise though, this looks fantastic!

components/dialog/index.css

rise-erpelding · 2025-07-01T18:16:10Z

components/dialog/index.css

 	}

+	.spectrum-Dialog-heading {
+		margin-block-end: var(--spectrum-standard-dialog-spacing-title-to-description);


Do you want a mod here? var(--mod-standard-dialog-spacing-title-to-description, var(--spectrum-standard-dialog-spacing-title-to-description))? I guess this was just moved up from below though maybe?

Although maybe more importantly, am I missing the spot where this would take effect? I think this might be taken over by the margin-block-end: 0 in the container query in every case 🤔

Or maybe we don't need this? You've already got a nice gap between the heading and the header content wrapper, and the header has its own margin-block-end that would provide space between the it and the dialog content if the header content wrapper wasn't there.

We still need this! This margin-block-end gets added for the default dialogs when the grid layout changes in smaller screens:

I wasn't going to add a mod, only because it wasn't there before, but that mod already exists, so I don't see a problem adding it here.

rise-erpelding · 2025-07-01T19:24:21Z

components/dialog/stories/dialog.test.js

+		"width": "100%", // forces the data-inner-container to be full width so the container query doesn't break.
+	},
+	containerStyles: {
+		"width": "100%", // forces the data-outer-container to be full width so the container query doesn't break.


Oh noes! I believe it's the first one (the inner-container) that's creating a horizontal scrollbar at the bottom in the default story (but not the test):

I did play with it for a bit and something like -webkit-fill-available does work rather than 100%, but doesn't work in Firefox.

We could also try using isChromatic for this? I don't know if it's cool to use that for components. But it'd look something like

import isChromatic from "chromatic/isChromatic"; // all the other stuff... wrapperStyles: isChromatic ? { "background-color": "var(--spectrum-gray-50)", "width": "100%", // forces the data-inner-container to be full width so the container query doesn't break. } : {}, // ...all the rest of the stuff

There might be a better solution than that also!

Ok and one more thing on this: would we want to refactor to use inline-size instead of width? I know using logical properties for the storybook wrapper styles is a little less critical than in the styles we ship, so I definitely don't consider that blocking at all.

marissahuysentruyt · 2025-07-07T16:34:39Z

components/dialog/stories/dialog.test.js

+			...(showTestingGrid && { "inline-size": "100%" }), // Applies conditional styles based on showTestingGrid- forces the data-inner-container to be full width so the container query doesn't break.
+		},
+		containerStyles: {
+			...(showTestingGrid && { "inline-size": "100%" }), // forces the data-outer-container to be full width so the container query doesn't break.


@rise-erpelding What do you think about this approach instead? I didn't notice the horizontal scroll before, but I believe it's gone now everywhere. Let me know!

5t3ph

🎉 great work!

- adds container query for fullscreen/fullscreenTakeover dialogs - increases the max-inline-size for the dialog to 864px for query - moves fullscreen/fullscreenTakeover grid layout to container query

marissahuysentruyt self-assigned this Jun 26, 2025

marissahuysentruyt added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. wip This is a work in progress, don't judge. S2 Spectrum 2 labels Jun 26, 2025

marissahuysentruyt commented Jun 26, 2025

View reviewed changes

components/dialog/index.css Show resolved Hide resolved

marissahuysentruyt changed the title ~~Marissahuysentruyt/css 1092 dialog header flexibility~~ refactor(dialog): add more header flexibility Jun 26, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1092-dialog-header-flexibility branch from cb5d59b to a2205c6 Compare June 27, 2025 21:12

marissahuysentruyt commented Jun 27, 2025

View reviewed changes

components/dialog/index.css Show resolved Hide resolved

marissahuysentruyt removed the wip This is a work in progress, don't judge. label Jul 1, 2025

marissahuysentruyt commented Jul 1, 2025

View reviewed changes

marissahuysentruyt added the run_vrt For use on PRs looking to kick off VRT label Jul 1, 2025

marissahuysentruyt marked this pull request as ready for review July 1, 2025 13:22

marissahuysentruyt added the ready-for-review label Jul 1, 2025

rise-erpelding self-requested a review July 1, 2025 17:22

rise-erpelding requested changes Jul 1, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-1092-dialog-header-flexibility branch from ced82f0 to 00bcbe5 Compare July 7, 2025 16:30

marissahuysentruyt commented Jul 7, 2025

View reviewed changes

rise-erpelding approved these changes Jul 7, 2025

View reviewed changes

5t3ph approved these changes Jul 10, 2025

View reviewed changes

marissahuysentruyt added 6 commits July 10, 2025 10:39

fix(dialog): condense t-shirt variables

c5dbad7

feat(dialog): add container query

af93887

- adds container query for fullscreen/fullscreenTakeover dialogs - increases the max-inline-size for the dialog to 864px for query - moves fullscreen/fullscreenTakeover grid layout to container query

chore(dialog): force dialog container to be full width for testing

37cfdec

chore(dialog): add changeset

e167ec6

test(dialog): render wrapper and container styles conditionally

f18ecb9

fix(dialog): add mod to dialog header selector

9a94096

marissahuysentruyt force-pushed the marissahuysentruyt/css-1092-dialog-header-flexibility branch from 00bcbe5 to 9a94096 Compare July 10, 2025 14:42

marissahuysentruyt merged commit f654c8d into spectrum-two Jul 10, 2025
15 checks passed

marissahuysentruyt deleted the marissahuysentruyt/css-1092-dialog-header-flexibility branch July 10, 2025 15:01

castastrophe mentioned this pull request Jul 25, 2025

ci: chromatic runs for S2 [CSS-1233] #4070

Merged

3 tasks

github-actions bot mentioned this pull request Jul 26, 2025

chore: release (next) #4073

Merged

castastrophe mentioned this pull request Aug 6, 2025

fix: minor linting updates #4113

Merged

11 tasks

This was referenced Aug 27, 2025

chore(popover): wrapper placement in story #4173

Closed

feat(storybook): chromatic a11y testing #4180

Closed

chore: updating lint-staged file inheritance order #4193

Closed

refactor(dialog): add more header flexibility #4000

refactor(dialog): add more header flexibility #4000

Uh oh!

Conversation

marissahuysentruyt commented Jun 26, 2025 • edited by rise-erpelding Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Jira/Specs

How and where has this been tested?

Jira/Specs

Validation steps

Additional validation

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

github-actions bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

dialog

Uh oh!

github-actions bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Branch preview

Uh oh!

Uh oh!

marissahuysentruyt Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

rise-erpelding Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rise-erpelding left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rise-erpelding Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

rise-erpelding Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

5t3ph left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marissahuysentruyt commented Jun 26, 2025 •

edited by rise-erpelding

Loading

changeset-bot bot commented Jun 26, 2025 •

edited

Loading

github-actions bot commented Jun 26, 2025 •

edited

Loading

github-actions bot commented Jun 26, 2025 •

edited

Loading

rise-erpelding Jul 1, 2025 •

edited

Loading

rise-erpelding left a comment •

edited

Loading

rise-erpelding Jul 1, 2025 •

edited

Loading