feat(dialog): use native HTML dialog element #2078
Conversation
Related to #1865.
🦋 Changeset detectedLatest commit: 6745875 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 |
✅ Deploy Preview for red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Size Change: +240 B (+0.12%) Total Size: 208 kB
ℹ️ View Unchanged
|
bennypowers
changed the title
There was a problem hiding this comment.
-
The close button's target looks pretty large. Can we use
--rh-length-xlfor the width and height of the touch target?
-
Can the slots use this spacing?
-
Is there a way to keep the close button fixed if the content has to scroll?
-
This comment might need to go back to design for a future feature update, but the close button isn't visible in the video dialog demo. (I can make an issue if design needs to tackle dark theme to fix this.)
…:RedHat-UX/red-hat-design-system into feat/dialog/use-native-dialog-element-v2
| align-items: center; | ||
| justify-content: center; | ||
| z-index: 500; | ||
| .visually-hidden { |
There was a problem hiding this comment.
Note
Not relevant to this PR, and please don't commit it to this branch, rather a separate PR.
I think it's time to move this and the previous rule into a shared stylesheet module, which would get adopted (via the static styles field) into every element.
There was a problem hiding this comment.
Could you expand on this how you'd recommend implementing this?
- Are you suggesting using CSS module scripts (limited browser support)?
- Would this go in RHDS? eg:
elements/shared/styles/visually-hidden.css - Would it go in pfe-tools? eg
@patternfly/pfe-tools/shared/styles/visually-hidden.css
Happy to get this going with some direction. TIA!
.changeset/loud-coins-shake.md
Outdated
|
|
||
| `<rh-dialog>`: Dialog now uses the native HTML `<dialog>` element internally. | ||
|
|
||
| Note: the `overlay` CSS shadow part is now deprecated in favor of the `--rh-dialog-backdrop-background-color` CSS custom property. |
There was a problem hiding this comment.
does the part still work? if not, this will need a major version bump
| Note: the `overlay` CSS shadow part is now deprecated in favor of the `--rh-dialog-backdrop-background-color` CSS custom property. | |
| Note: the `overlay` CSS shadow part is now deprecated in favor of the `--rh-dialog-backdrop-background-color` CSS custom property. It will still work, but you can expect the `overlay` part to be removed in a future version |
Also, please confirm @coreyvickery that this deprecation aligns with design expectations: namely that in future versions the only customizable aspect of the backdrop will be its color (including transparency) - and that the existing ability to customize any css property for the backdrop should be removed.
There was a problem hiding this comment.
does the
overlaypart still work?
Current version of overlay on uxdot is the overlay/backdrop of the dialog.
This PR's version keeps the overlay <div> and many of the styles (lines 146-149 in rh-dialog.ts and lines 35-44 in rh-dialog.css).
The only change is overlay is a transparent <div> overlaying the contents of the page. It's invisible to the user and customizable via the existing CSS part—should a user want to do that.
With this PR's upgrade to the native <dialog> element, the native ::backdrop property is being used alongside a customizable CSS variable.
There was a problem hiding this comment.
Added Benny's note about removing overlay in a future version in 34d7598.
There was a problem hiding this comment.
Paging Mr. @coreyvickery. 😇
See Benny's questions about overlay/backdrop and --offset vars below.
| block-size: 100%; | ||
| content: ''; | ||
| inline-size: 100%; | ||
| inset-block-start: calc(-1 * var(--offset-top)); |
There was a problem hiding this comment.
--offset-top should either be --_offset-top or --rh-dialog-offset-block-start, depending on whether @coreyvickery intends for the offset to be user-customizable.
There was a problem hiding this comment.
Indeed, the following ("public") variables currently exist on uxdot and are in production—albeit not documented:
--offset
--offset-top
--offset-rightThat leaves us with a decision:
- Do we change these variables as suggested?
- A breaking change
- Or, do we leave these variables as is (and possibly document them)?
There was a problem hiding this comment.
I think we need a patch changeset but one that clarifies that these names were always supposed to be private. WDYT @zeroedin? cc @zhawkins @OneEightyFirst
There was a problem hiding this comment.
I agree with @bennypowers here; if undocumented, that tells me the intention from the start was that they were supposed to be private, and we could likely get away with a patch.
There was a problem hiding this comment.
If we wanted to be super conservative, the other option might be to do something like:
#selector {
/* @deprecated: --offset-top and will be removed in a future release */
margin-block-start: var(--offset-top, var(--_offset-top)); 👈
}
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
|
I'll be making dialog docs updates in this branch/PR this week! Please don't merge before they're done. |
What I did
<dialog>element.accessible-labelproperty<dialog>elementTesting Instructions
variant="small"etc andposition="top")Notes to Reviewers
Docs updates:
Closes #1242 and #1238.