fix(menu): drill-in disabled menu item chevron #2199
Conversation
|
🚀 Deployed on https://pr-2199--spectrum-css.netlify.app |
| @@ -342,7 +342,12 @@ DrillInSubmenu.args = { | |||
| isDrillIn: true, | |||
| isOpen: true, | |||
| }, | |||
| { label: "Select and Mask..." }, | |||
| { | |||
A drill-in set to "open" was no longer showing the disabled color for the chevron icon. Moves disabled CSS after other CSS, which resolves this issue. Also includes this in the existing Storybook example for drill-in. Fixes #2176
pfulton
force-pushed
the
jawinn/css-594-fix-disabled-menu-item
branch
from
d7a5fc6
to
d015a16
Compare
| &:hover { | ||
| cursor: default; | ||
|
|
||
| .spectrum-Menu-itemIcon { |
There was a problem hiding this comment.
Hi, sorry to revive this issue, but this implementation doesn't work with our API in SWC. The chevron is implemented differently than the other icons that can be slotted into the menu item, so I think the class .spectrum-Menu-chevron would need specification for being disabled, as well, in order to receive styling. Please look here to see the difference between an icon and the chevron. The icon has disabled styling, but the chevron still doesn't.
There was a problem hiding this comment.
I took a look at this and tested out added CSS with this draft PR https://github.com/adobe/spectrum-css/pull/2550/files#diff-3b8d6209262a97e225fba580d5a675267ced29e23aadabdcc2cb73332bf11d94 , but in looking at it further I'm not sure if this extra selector is something we should add on our end? Your thoughts @pfulton ? SWC seems to have diverged from how we use our CSS. On our end, the .spectrum-Menu-itemIcon class is a generic one applied to ALL menu item icons, including the workflow icon, the collapsible chevron, the drill-in chevron, and the checkmark. Can SWC also apply this class or these styles to all icons?
There was a problem hiding this comment.
We can't apply that class, but we can make apply CSS as is appropriate to our context if it doesn't work for CSS to apply this rule here. Ensuring the "system icons" and "consumer icons" are both covered by this styling works a bit different in our world, for sure.
Separately, I'm never sure if I'm working from latest in the CSS public documentations, but can you apply is-disabled to one of the Menu Items in this story?
We'd love to see CSS applying the disabled text color to that "value" content as well, here.
There was a problem hiding this comment.
Separately, I'm never sure if I'm working from latest in the CSS public documentations, but can you apply
is-disabledto one of the Menu Items in this story? We'd love to see CSS applying the disabled text color to that "value" content as well, here.
In PR #2579 I've updated the Storybook templates with that increased coverage and have added CSS for the "value" text to use the disabled content color.
|
Hi, while you're doing this work for the chevron, could you also add styling for a disabled |
Description
A drill-in set to "open" was no longer showing the disabled color for the chevron icon. Moves disabled CSS after other CSS, which resolves this issue.
Also adds this case in the existing Storybook example for drill-in.
Fixes #2176
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation Steps
Regression testing
Validate:
Screenshots
To-do list
I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨