react icon indicating copy to clipboard operation
react copied to clipboard

ActionList: Fix leaky disabled description styles

Open siddharthkp opened this issue 1 year ago • 3 comments

  • Reported in https://github.com/github/collaboration-workflows-flex/issues/927

Problem

Description in a disabled item should not have color: fg.muted but the disabled color. We set this by adding the disabled color on the item and adding color:inherit on the description. Source

const styles = {
  'li[aria-disabled="true"] &': { color: 'inherit'  }
}

return <Box as="span" sx={styles}/>{props.children</Box>

This feels safe because we are only targeting Description inside li[aria-disabled=true]. This is the generated css:

li[aria-disabled="true"] .Box-sc-g0xbh4-0 {
    color: inherit;
}

The trouble here is that the generated className for '&' is not the className just for ActionList.Description but is passed to multiple elements within ActionList.Item. 🤔

This really threw me off!

the className Box-sc-g0xbh4-0 is present on multiple elements

Solution

As a quick fix, I added a data-component to make the selector more specific. Expecting no visual changes, only changes in the generated css.

Before
li[aria-disabled="true"] .Box-sc-g0xbh4-0 {
    color: inherit;
}
After
li[aria-disabled="true"] .Box-sc-g0xbh4-0[data-component="ActionList.Description"] {
    color: inherit;
}

Rollout strategy

  • [x] Patch release
  • [ ] Minor release
  • [ ] Major release; if selected, include a written rollout or migration plan
  • [ ] None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

  • [ ] Added/updated tests
  • [ ] Added/updated documentation
  • [x] Added/updated previews (Storybook)
  • [x] Changes are SSR compatible
  • [x] Tested in Chrome
  • [ ] Tested in Firefox
  • [x] Tested in Safari
  • [ ] Tested in Edge
  • [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

siddharthkp avatar May 07 '24 13:05 siddharthkp

🦋 Changeset detected

Latest commit: e995bb9b9eed1356669ec3a614d1575e6fc1767b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

changeset-bot[bot] avatar May 07 '24 13:05 changeset-bot[bot]

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 88.07 KB (+0.04% 🔺)
packages/react/dist/browser.umd.js 88.43 KB (+0.06% 🔺)

github-actions[bot] avatar May 07 '24 13:05 github-actions[bot]

wanted to ask if this is possible to capture in VRT or nah? (Or is it already?)

Maybe but not a clean one. The instance where this happens in gh/gh is also not the best use case for ActionList

siddharthkp avatar May 08 '24 10:05 siddharthkp

:wave: Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/false

primer-integration[bot] avatar Jul 11 '24 06:07 primer-integration[bot]