Button: Make button visuals overridable by sx

[broccolinisoup]

As described in the issue, a regression introduced in #3447 where the icon colors became not overridable by the sx prop in Button.

This issue originally goes back to introducing data-attributes selectors to Button (#2792) and it came with some challenges with overriding styles with sx. At the time, we wrote a custom CSS selector to take data attributes into account while applying the sx styles. However, the new icon styles that are introduced in #3447, are not included in the custom selectors.

Note: We won't have to use this css selectors when we move to css modules ✨ but for now, it seems to be doing what we want.

All that said, I am always open to any kinds of feedback 🙏🏻

Closes https://github.com/github/primer/issues/2576

Screenshots

Before

After

Merge checklist

• Added/updated tests
• Added/updated documentation
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: ff416d3

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.74 KB (+0.05% 🔺)
dist/browser.umd.js	105.32 KB (+0.05% 🔺)

[broccolinisoup]

These lines are repetitive and I am happy to take into a loop but thought keeping this way makes them more readable. Let me know if you have any feedback!

[joshblack]

Thanks @broccolinisoup! Will take a look at the tests 👀

Wanted to ask you a quick question, is it possible at all to use a CSS Custom Property when color is set to coordinate this instead of the stuff we're doing in JS? Something like:

const ButtonComponent = forwardRef(({children, sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => {
  let sxStyles = sxProp

  // grap the button props that have associated data attributes in the styles
  const {block, size, leadingIcon, trailingIcon, trailingAction} = props
  const style = {};

  if (sxProp !== null && Object.keys(sxProp).length > 0) {
    sxStyles = generateCustomSxProp({block, size, leadingIcon, trailingIcon, trailingAction}, sxProp)

    if (sxProp.color) {
      style['--button-color'] = sxProp.color;
    }
  }

  return (
    <ButtonBase ref={forwardedRef} as="button" sx={sxStyles} type="button" style={style} {...props}>
      {children}
    </ButtonBase>
  )
}) as PolymorphicForwardRefComponent<'button', ButtonProps>

And then in the styles we use it like:

// ...

'[data-component="leadingVisual"], [data-component="trailingVisual"], [data-component="trailingAction"]': {
  color: `var(--button-color, ${theme?.colors.fg.muted})`,
},

// ...

Or would this not really work out the same way? Just was thinking of a simple way we could explicitly opt in things to this value but totally understand if it's not really the same thing at all as the custom sx prop seems to be doing a ton.

[broccolinisoup]

Hey @joshblack that totally works for the color 🎉 I wasn't very happy how the generateCustomSxProp function got more complex and unreadable but couldn't think of a different way. I appreciate this so much 🙏🏻 I'll definetely update the implementation just a few things clarify around this:

• I was thinking if we can apply the custom css prop solution you suggested to the rest of the styles that uses data attributes to simplify the custom sx prop function. However, as far as I understand, this might be a bit challenging for the rest of the styles that uses data attribute.

For example to make sure sx styles are applied properly to the size="small" button that uses data-attribute for setting styles, seems like we need to create css var for each attribute that we are overriding i.e. font-size, padding etc. Is this what you are reading as well or do I miss something here?

• In terms of the jest tests, I was using toHaveStyle for expect but seems like that function has some issue with the css variables (some related open issues I found: issue1, issue2) - Do you have any recommendation to test these styles in jest? I am also happy to add test stories for each case for VRT. Let me know what you think!

Thanks again, I appreciate your input 🙏🏻

[joshblack]

Would love to see how this works out over in dotcom 🤞 Let me know if I missed anything since the last time we spoke.

[broccolinisoup]

@joshblack All looking good! Checks are green and tested in review lab to see the button that is originally reported as an issue

https://github.com/github/github/pull/291667

I'm merging this - let me know if you have any concern!