fix(accordion): hover colors #2068
Conversation
🦋 Changeset detectedLatest commit: 3d3f330 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: +38 B (+0.02%) Total Size: 207 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
-
I can see the hover background color in the color context demo, but it's not showing up in any other demo.
-
Did the div that was removed allow the red line to extend to the panel header when it's expanded?
- On hover, the accordion in dark theme shows black text.
|
looks like maybe some ssr problems |
|
@marionnegp PTAL, thanks |
There was a problem hiding this comment.
The changes to previous comments look good! I noticed a couple other things that I might have missed last time.
-
When any of the accordion panels are expanded, the last accordion also shows a dropshadow. There shouldn't be a dropshadow on a panel unless it's expanded.
-
When you first land on the color context demo, the text in an expanded panel is black for the dark theme accordions. I'm able to resolve it by viewing the accordion in a light color palette and then clicking back to a dark color palette, so it might just be a demo error. Noting it in case this confuses UX dot users.
|
let's do the drop shadow thing in a separate issue - there was confusion about this earlier in the quarter and i want to make sure we're all on the same page with regards to the context/ssr issues, i added another workaround, so please take a look |
|
Should we add a changeset line about this PR correcting the regression in |
There was a problem hiding this comment.
According to the docs, the focus states still need some love:
- Background color needs to be changed / applied on focus
- Ensure blue outline on focus across browsers
Focus state in Microsoft Edge:
Focus state in Safari:
Note that in Safari, users could easily confuse which item is currently focused.
There was a problem hiding this comment.
The font weight of the accordion panel headers looks different in Safari and Firefox then it does in Chrome.
Chrome:
Firefox:
Safari:
In fact manually changing Chrome's font weight to 500 and 700 does no visual change.
red-hat-design-system/elements/rh-accordion/rh-accordion-header.css
Lines 128 to 130 in 8ec2cc4
red-hat-design-system/elements/rh-accordion/rh-accordion-header.css
Lines 62 to 71 in 8ec2cc4
I believe this has to do with the fonts stack we are loading and Chrome's difference in how it renders weights that aren't loaded for the stack. Where as I believe Safari and seemingly Firefox too will synthesize it if it is missing.. at least I believe I remember seeing that somewhere was the case for Safari although a quick search didn't return proof to that respect.
red-hat-design-system/docs/styles/fonts.css
Lines 25 to 39 in 8ec2cc4
We do not load a 700 weight for RedHatText in our fonts.css file. We do however load a 700 weight for RedHatDisplay.
Are we sure we want to be using RedHatText for this header?
red-hat-design-system/docs/styles/fonts.css
Lines 17 to 23 in 8ec2cc4
If so, we'll need to modify the font stack to accommodate the 700 weight for body text. Also that said tokens for body text only go to 500 so likely this isn't the font family we want here.
Quite possible we might just want to move this to another issue and fix outside this PR.
Otherwise the rest of the changes in the PR LGTM. @bennypowers at some point a small walk through on the lit ssr workaround maybe warranted just to make sure I have the correct understanding. As I understand it we are triggering a update with the prop change there to ensure the consumer fires it request event?
Update: moved to its own issue: #2083
|
yes, @zeroedin : in my experiments, I found that no amount of i'm looking forward to lit-ssr supporting this stuff so we can jettison our custom code |
Closes #2065
What I did
Testing Instructions