Overlay component #1998
Overlay component #1998
Conversation
🦋 Changeset detectedLatest commit: ea44985 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Added to the Primer Issue: https://github.com/github/primer/issues/426 |
bolonio
changed the title
| overflow-y: auto; | ||
| padding-top: 0; | ||
| overflow-y: scroll; | ||
| scrollbar-width: thin; |
| top: 4px; | ||
| } | ||
|
|
||
| @mixin Overlay-backdrop() { |
There was a problem hiding this comment.
Should we use a .Overlay-backdrop class instead of a mixin? We would have to add two classes when used:
- class="Overlay-backdrop--center"
+ class="Overlay-backdrop Overlay-backdrop--center"but we should be able to save some file size because these "base styles" would not get repeated for every variation.
There was a problem hiding this comment.
I see what you're saying.. it's a little more complicated though because we also have Overlay-backdrop--transparent(). So instead of overriding styles in each backdrop class .Overlay-backdrop--anchor I extracted it into a mixin 🤔 I think if I went back to classes, we would have to include more conditional logic within each class which might add more complexity to the selectors.
I think this CSS is a little more complex than I would like it to be (using mixins so extensively) but the responsive behavior is complex 😂
| left: 0; | ||
| z-index: 999; | ||
| display: flex; | ||
| background-color: var(--color-neutral-muted); |
There was a problem hiding this comment.
This is more a design question, but there is also the --color-primer-canvas-backdrop that is meant for the backdrop of modal/dialogs. It is darker and less transparent. So not sure if --color-neutral-muted is intentionally used here.
There was a problem hiding this comment.
It is intentionally used here.. I either pulled this from @vdepizzol's designs or from the PRC implementation. Perhaps we will need to update the primitive to match this new design decision (or deprecate that token and make a new one.)
There was a problem hiding this comment.
Ok, yeah.. currently --color-primer-canvas-backdrop gets used for details-overlay-dark and the "old" SelectMenu when in narrow. Which we might wanna keep as is, for now.
IMO, the --color-neutral-muted used as a backdrop feels a bit too transparent and doesn't direct the attention enough to the dialog because you can still see the rest of the the UI. Anyways, something we can reconsider later too. I still have to see it in a live example.
Co-authored-by: simurai <[email protected]>
…ithub.com/primer/css into accessibility/issues/925/dialog-component
…ithub.com/primer/css into accessibility/issues/925/dialog-component
What are you trying to accomplish?
We, the @github/accessibility team, have been working on a new Dialog Component that should become a Primer Component.
Note: minor class changes will need to happen when the PVC Dialog is upstreamed into PVC. Alternatively, Dialog will use the new Overlay component under the hood (private API).
This PR includes:
src/overlay/overlay.scssdocs/src/stories/components/Dialog/Dialog.stories.jsxdocs/content/components/dialog.md(removed from nav to promote PVC)The implementation of the New Experimental Dialog Component can be found in this PR:
What has changed?
BEFORE
AFTER
Are additional changes needed?
No, this PR should be ok to ship as is. 🚢