-
Notifications
You must be signed in to change notification settings - Fork 198
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
feat(actiongroup)!: migrate to S2 #2453
Conversation
2869e63
to
f55489b
Compare
f55489b
to
71d3327
Compare
71d3327
to
56d4e45
Compare
File metricsSummaryTotal size: 3.89 MB*
Detailsactiongroup
tokens
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-2453--spectrum-css.netlify.app |
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.
Looked good to me, just had a question.
components/actiongroup/index.css
Outdated
|
||
/* account for button border */ | ||
--spectrum-actiongroup-horizontal-spacing-compact: calc(-1px * var(--spectrum-spacing-50)); | ||
--spectrum-actiongroup-vertical-spacing-compact: calc(-1px * var(--spectrum-spacing-50)); |
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.
instead of -1px
do we want to use the button border token?
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.
Could you also explain what these are doing? it looks like previously the compact buttons had negative margin but I am not seeing this after your changes.
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.
This is a really good question. Originally I moved this code here from the Express theme file. But when I looked at the Express actiongroup in prod, there's also not any negative margins on those buttons despite using this same formula. Turns out that this calculation, which simplifies into `calc(-1px * 2px) is an invalid property value:
This is because when using multiplication with the calc()
function, it's required that one of the numbers be unitless. So we functionally have margins set to zero for the compact variant in Express. Given that the S2 spec defines space between buttons in both the default and compact variant, we should be good to remove all these margin values. ✨ I'll push up that change.
In case it helps you test, I tested this change by checking the values in the PR deploy with dev tools but also by screenshotting the variants and pulling them into Figma to be able to see that the pixel values match the variants (2px spacing for compact and 8px for default).
56d4e45
to
1ff683e
Compare
BREAKING CHANGE
1ff683e
to
8f59fe6
Compare
| `--mod-actiongroup-button-spacing-reset` | | ||
| `--mod-actiongroup-gap-size-compact` | | ||
| `--mod-actiongroup-horizontal-spacing-compact` | | ||
| `--mod-actiongroup-horizontal-spacing-regular` | | ||
| `--mod-actiongroup-vertical-spacing-compact` | |
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.
Is it ok to remove these mods since this is a breaking change, or is there a different way we need to do this?
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.
Yes, I think this is an ok change to make!
✅ approved by design via Slack. |
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
Description
Migrate actiongroup to S2 design.
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:
To-do list