feat(disclosure): add <rh-disclosure>
#2043
Conversation
🦋 Changeset detectedLatest commit: 4f92e56 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: +2.23 kB (+1.07%) Total Size: 210 kB
ℹ️ View Unchanged
|
|
I'm curious why you opted to make Late-2000s haute cuisine trends aside, it would be a touch annoying to order a burger and have the patty, buns, veg, and sauce, all come in separate containers with assembly instructions stapled to the bag. As custom-element authors, we should have the confidence to define our own behaviours. Tn other words, if if downstream teams "need" lightdom details (which is in my view an antipattern) what about making it opt-in? Elaborating on my position: we should make the maximum effort to avoid requiring lightdom which performs the same function. This is to gain maximum benefit from custom elements and shadow dom, namely encapsulation and interoperability. There have been and will likely continue to be instances where the state of the art gives us no choice, for example I suspect that pfe's radio and checkbox elements will have to manage their own lightdom, although I'm concerned that this might break under the control of "VDOMs" like react. In the future, when reference targets, aria IDL attributes, more ElementInternals apis, and improvements to In the case of disclosure though, I haven't yet seen the show-stopping must-have requirement that the So all that in mind, I challenge you to imagine a disclosure element in which |
|
consider <sl-details summary="Toggle Me">
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna
aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
</sl-details>
|
So that it works with JavaScript disabled, avoids FOUC, and drives adoption. (see this draft PR demo of Universal Nav with shadowdom'ed We thought that rh-disclosure might be used for Your opt-in example is indeed interesting and something I'd be willing to pursue if we decide that path is our way forward. If that's not amenable, then I would probably do something similar to what |
There was a problem hiding this comment.
-
Light theme focus ring color should use
--rh-color-interactive-primary-default-on-lightfor active and focus states.
-
When the disclosure is expanded, it's showing a 24px space between the summary slot and the body content. According to our Figma file, it should be 32px (
--rh-space-2xl). The Figma file's been updated more recently than the disclosure docs page, so apologies for the inconsistency there!
@marionnegp I used |
There was a problem hiding this comment.
Focus ring changes look good!
I wanted to check the focus ring in the nested disclosures, so I wrapped that demo with <rh-surface>. I noticed that not everything ended up being themed correctly. (Links aren't light blue and the third nested disclosure's caret icon is black.) Is this expected based on how <rh-surface> works (maybe I used it wrong), or is it a bug?
Links aren't light blueThis is expected behavior. It's the responsibility of the page author to style elements in the lightdom. eg: The third nested disclosure's caret icon is blackI could not reproduce this. See the screenshot below. The caret color stays the same across all disclosures and doesn't change if one is nested. Double check if it's still happening for you. If it is, kindly share some steps to reproduce and I will be happy to look into it. Thanks for all your feedback!
|
There was a problem hiding this comment.
Sorry one more thing! Disclosure should use the same interaction state styles as accordion.
| @@ -0,0 +1,96 @@ | |||
| :host { | |||
| /* stylelint-disable-next-line rhds/token-values */ | |||
There was a problem hiding this comment.
why do we need to disable this lint rule here? Shouldn't this use the computed value?
There was a problem hiding this comment.
When a <rh-disclosure> is not wrapped by a <rh-surface>, the computed value is undefined.
This results in incorrect colors (no brand colors, no blue interactive colors—just the browser's default [transparent]).
You can see what happens if we remove the fallback colors by visiting the demo on the DP, inspect element on the host, then remove the fallback for the :host border.
So, what's the best way to tackle this?
- Add an
<rh-surface>to the shadowdom every time this issue appears - Keep the fallbacks, keep the
/*stylelint-disable... */ - Something else...?
There was a problem hiding this comment.
Should disclosure have a color palette (ie be a provider)? cc @coreyvickery @marionnegp
There was a problem hiding this comment.
If, like cta, the intention is for disclosure to always only ever be a consumer, then yes, surface or another palette container is necessary (for the time being)
There was a problem hiding this comment.
I think it should be a provider, like accordion. I can see users wanting to slot similar content into a disclosure.
| &:active, | ||
| &:focus { | ||
| outline: var(--rh-border-width-md, 2px) solid; | ||
| /* stylelint-disable-next-line rhds/token-values */ |
|
|
||
| &:before { | ||
| content: ''; | ||
| /* stylelint-disable-next-line rhds/token-values */ |
| @@ -0,0 +1,26 @@ | |||
| rh-disclosure:not(:defined) { | |||
| /* stylelint-disable-next-line rhds/token-values */ | |||
There was a problem hiding this comment.
why do we need to override the lint rule here? shouldn't we use the computed value?
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
What I did
<rh-disclosure>elementTesting Instructions
<rh-disclosure>demo on the DP.To do's
main👉staging/cubone+ diffNotes to Reviewers
This element is lightdom based. We're planning to use it standalone as<rh-disclosure>and also using it in<rh-navigation-XYZ>for dropdowns.Shout out to @zeroedin for the initial concept.
Closes #1293.