Uh oh!
There was an error while loading. Please reload this page.
Update hover styles for ActionList item - #3815
Conversation
🦋 Changeset detectedLatest commit: 1bee44a 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 |
89a54ff to
1069665Comparesize-limit report 📦
|
1b16d31 to
1be4fd3Compare
langermank
left a comment
There was a problem hiding this comment.
Nice! Just one comment about the color token.
| ':hover:not([aria-disabled])': { | ||
| backgroundColor: `actionListItem.${variant}.hoverBg`, | ||
| color: getVariantStyles(variant, disabled).hoverColor, | ||
| boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.colors.border.muted}`, |
There was a problem hiding this comment.
| boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.colors.border.muted}`, | |
| boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.actionListItem.default.activeBorder}`, |
I'm not sure if that syntax is right, maybe its:
boxShadow: `inset 0 0 0 max(1px, 0.0625rem) actionListItem.default.activeBorder`,
Regardless, this should use the ActionList specific color token which you can also see in the Primer View Components version 😄
There was a problem hiding this comment.
I think with the sx prop it needs to either be just the token or uses that theme from context, potentially? So with the new token it would be:
| boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.colors.border.muted}`, | |
| boxShadow: `inset 0 0 0 max(1px, 0.0625rem) ${theme?.colors.actionListItem.default.activeBorder}`, |
There was a problem hiding this comment.
Me and @broccolinisoup went back and forth with the colors and got confused! Thanks for this input. I have updated the PR!
joshblack
left a comment
There was a problem hiding this comment.
LGTM after the token change! 🥳
15597d5 to
30f9691Compare
broccolinisoup
left a comment
There was a problem hiding this comment.
Looks really good! 🚀
Note: I am not sure if it is just me but the after video in the PR description, doesn't render for me. In case you would like to update for future references 😊
langermank
left a comment
There was a problem hiding this comment.
Looks perfect! Thanks for making that change 😄
Closes#2479
In dark high contrast theme, the
ActionMenushows no hover color since the hover color and the menu overlay color are the same in this particular theme. However, if we refer to theview_componentsimplementation, we see that they have a hover shadow as well which does the job pretty well in this theme. I've added the same shadow styles toActionListItem. This looks great as the fix for our bug without regressing during other cases.Before
Screen.Recording.2023-10-13.at.2.28.38.pm.mov
After
Screen.Recording.2023-10-13.at.2.30.48.pm.mov
Note
This fixes Light High contrast mode ActionItems as well.
Rollout strategy
Testing & Reviewing
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.