chore(dialog): z-index for dialog docs #2855

marissahuysentruyt · 2024-06-21T20:21:03Z

Description

Currently on the static docs site, the dialogs don't really function properly (the default dialog does however, work properly in Storybook. If you close any open dialog variant, and try to reopen it, it is "stuck" underneath spectrum-Underlay. I believe this is because a z-index: 1 !important is overriding other z-index: 2s for spectrum-Modal.

This PR adds a new .spectrum-CSSExample-modal class to mimic .spectrum-Modal-wrapper with a z-index that should position it above .spectrum-Underlay. It also adds/removes classes in dialog.yml to better implement the open/close behaviors.

Note: The hero, no-divider, and dismissible dialogs are now closed on page load, as opposed to being open previously.

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.

Validation steps

@jawinn

Pull down the branch
Run yarn run dev and visit the dialog docs page
Verify the no-divider, dismissible, and hero variants are closed on page load
Test all closed dialogs on the page:
- opening the dialog should add the is-open class to spectrum-Modal and spectrum-Modal-wrapper
- once the dialog is open, any buttons (like the close button or a button group) should be clickable/focusable
- the z-index for spectrum-CSSExample-modal should be 2 !important to ensure it remains above the z-index: 1 !important set on spectrum-Underlay.
- closing the dialog should remove is-open from those same modal elements

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

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-06-21T20:21:07Z

⚠️ No Changeset found

Latest commit: 5ea1e1f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-06-21T20:24:04Z

🚀 Deployed on https://pr-2855--spectrum-css.netlify.app

github-actions · 2024-06-21T20:24:22Z

File metrics

Summary

Total size: 4.66 MB^*
Total change (Δ): ⬆ 0.20 KB (0.00%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
site	14.58 KB	⬆ 0.06 KB

Details

site

File	Head	Base	Δ
index-base.css	14.58 KB	14.52 KB	⬆ 0.06 KB (0.38%)
index-vars.css	14.58 KB	14.52 KB	⬆ 0.06 KB (0.38%)
index.css	14.58 KB	14.52 KB	⬆ 0.06 KB (0.38%)
metadata.json	5.67 KB	5.63 KB	⬆ 0.03 KB (0.59%)

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

jawinn

I validated on the PR site and this fixes the issues on the docs page for Dialog 🎉

There may be some changes we should make in the future to be more in line with SWC or to be clear about how source order affects both Underlay and Dialog being used together:

Thoughts on the z-indexes:

I poked around a little bit and I see what is going on now, and am wondering if this should be looked into as a followup issue. The original display issue stems from both .spectrum-Modal and .spectrum-Underlay having z-index: 1, and whether one displays on top of the other depends on the source order.

In the original prod issue, there is only a single underlay, later on in the HTML source (after the Dialog markup). In Storybook, the underlay markup appears before the Dialog, which is why it looks fine; you can test to see the bug behavior there by dragging the underlay to be after the Dialog in the DOM inspector.

I looked at Spectrum Web Components, and they have the underlay within what we call .spectrum-Modal-wrapper (their sp-dialog-base uses those styles), also before the dialog modal:

This brings up a few questions for me:

If we want to allow underlay to be later in the source, should .spectrum-Modal have z-index: 2?
If underlay should always be before the modal, should we have multiple underlay divs for each one of these examples, and the JS should be targeting those? And should that be documented?
If so (# 2), should we be more in line with SWC and have the underlay nested within .spectrum-ModalWrapper?

rise-erpelding

This does indeed fix the issue! I’m honestly a little puzzled because it looks like .spectrum-Modal does have a z-index: 2 in the code but every time I inspect (and also in the dist), it has a z-index: 1. Seems like something in the build is changing the z-index, maybe? 🤔

If the modal is expected to always have z-index: 1 then I would say let’s not throw !important in with z-index: 2 !important but since I'm seeing z-index: 2 for .spectrum-Modal and !important is already pretty liberally sprinkled in with the CSSExample classes, this feels like the right choice.

+1 to Josh’s point that underlay isn’t causing issues elsewhere because of the stacking order (looks like from his comment he’s also seeing .spectrum-Modal having z-index: 1), this would be a good thing to clarify at some point.

- added `.spectrum-CSSExample-modal` to mimic `.spectrum-Modal-wrapper` with z-index that should position it above `.spectrum-Underlay` - adds/removes classes in dialog.yml to better implement the open/close behaviors. The hero, no divider, and dismissible dialogs are now closed on page load.

marissahuysentruyt marked this pull request as ready for review June 24, 2024 20:04

marissahuysentruyt requested review from jawinn and rise-erpelding June 24, 2024 20:49

jawinn approved these changes Jun 25, 2024

View reviewed changes

marissahuysentruyt requested review from pfulton and castastrophe June 25, 2024 20:11

rise-erpelding approved these changes Jun 26, 2024

View reviewed changes

castastrophe approved these changes Jun 27, 2024

View reviewed changes

castastrophe force-pushed the marissahuysentruyt/css-812-dialog-z-index-bugs branch from dd31ef2 to 01bc3e3 Compare June 27, 2024 16:02

castastrophe added skip_vrt Add to a PR to skip running VRT (but still pass the action) size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. ready-to-merge labels Jun 27, 2024

castastrophe changed the title ~~fix(dialog): z-index for dialog docs~~ chore(dialog): z-index for dialog docs Jun 27, 2024

castastrophe added the documentation Because documentation is important and shouldn't be broken label Jun 27, 2024

castastrophe enabled auto-merge (squash) June 27, 2024 16:03

castastrophe force-pushed the marissahuysentruyt/css-812-dialog-z-index-bugs branch from 01bc3e3 to 5ea1e1f Compare June 27, 2024 17:59

castastrophe merged commit 1ea5274 into main Jun 27, 2024
11 checks passed

castastrophe deleted the marissahuysentruyt/css-812-dialog-z-index-bugs branch June 27, 2024 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(dialog): z-index for dialog docs #2855

chore(dialog): z-index for dialog docs #2855

marissahuysentruyt commented Jun 21, 2024 •

edited by jawinn

Loading

changeset-bot bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

site

jawinn left a comment •

edited

Loading

rise-erpelding left a comment •

edited

Loading

chore(dialog): z-index for dialog docs #2855

chore(dialog): z-index for dialog docs #2855

Conversation

marissahuysentruyt commented Jun 21, 2024 • edited by jawinn Loading

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented Jun 21, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Jun 21, 2024 • edited Loading

github-actions bot commented Jun 21, 2024 • edited Loading

File metrics

Summary

site

jawinn left a comment • edited Loading

Choose a reason for hiding this comment

Thoughts on the z-indexes:

rise-erpelding left a comment • edited Loading

Choose a reason for hiding this comment

marissahuysentruyt commented Jun 21, 2024 •

edited by jawinn

Loading

changeset-bot bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

jawinn left a comment •

edited

Loading

rise-erpelding left a comment •

edited

Loading