-
Notifications
You must be signed in to change notification settings - Fork 639
Button style enhancement #2792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Button style enhancement #2792
Conversation
🦋 Changeset detectedLatest commit: 8b8cf1f 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 |
size-limit report 📦
|
src/Button/styles.ts
Outdated
@@ -103,15 +102,15 @@ export const getVariantStyles = (variant: VariantType = 'default', theme?: Theme | |||
}, | |||
}, | |||
invisible: { | |||
color: 'accent.fg', | |||
color: 'btn.text', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random question, if something is currently theming/relying on btn.text
would this change impact them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshblack can you give me an example of what you're thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@langermank I probably should have used btn.hoverBg
or btn.selectedBg
as a better example 😅 It seemed like if someone created a custom theme like:
const customTheme = deepmerge(theme, {
colors: {
btn: {
hoverBg: 'var(--my-custom-color)',
},
},
})
Based on the example here: https://primer.style/react/theming#customizing-the-theme
That this might no longer work since that btn.hoverBg
token is no longer used in the hover styles. Hope that makes sense, let me know if I'm misunderstanding anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshblack I didn't know we offered theming options like that! That's kind of risky isn't it, and makes it near impossible for us to even rename things without any visual changes 😅 but yeah, you're right, it would break. Does that mean I need to move this to a major release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@langermank agreed, I think it brings in component tokens as part of the public API. Would an interim solution potentially be to set the value of these tokens to the new expected value? Not sure if the references work that way but wanted to throw it out there if so.
Also added this to that versioning doc under "theming": https://github.com/primer/react/blob/37cfd07fb1eef4c0655157a0c9025cec94abaed5/contributor-docs/versioning.md#changes so we can align on if this is part of semver or not for the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would an interim solution potentially be to set the value of these tokens to the new expected value?
Normally I would say yes, but this component was using generic color tokens here– not specific to Button. I think it would be weird to override accent.fg
with a different color 🤔 unless I'm not understanding correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@langermank was more-so thinking of stuff like:
btn.hoverBg
has the valueactionListItem.default.hoverBg
btn.selectedBg
has the valueactionListItem.default.activeBg
Note: there was one usage of btn.selectedBg
going to actionListItem.default.selectedBg
that I wasn't sure how to remediate
I think the big thing was just trying to see if we could keep the component tokens (btn.*
) the same in the source code but change their values in the theme, if possible. Totally get if this really isn't do-able though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshblack I'm reverting these changes for now and will add them back into the followup PR for our next major release 😄
…react into button-style-enhancement
Note: These are the non breaking changes from this PR
Improvements
block
prop for full widthalignContent
prop to align content to center or startcontrol
sizing CSS variable values (not using CSS vars just yet)height
over padding for more control over sizingVisual updates
Addresses a few things missing from this discussion: https://github.com/github/primer/discussions/1016
Closes:
Screenshots
Please refer to the comprehensive Storybook stories- this is just a small sample of some of the changes
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.