Skip to content
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

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

mdt2
Copy link
Collaborator

@mdt2 mdt2 commented Jan 22, 2024

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

  • In Storybook, actiongroup gap spacing matches the S2 spec
  • In Storybook, actiongroup has representation of all variants
  • In the docs site, actiongroup gap spacing matches the [S2 spec]
  • actiongroup no longer has theme-specific styles (when I removed the files completely, Storybook was throwing errors, so the empty files remain for now)
  • Design validation @mdt2
    • Specifically check if changes at XS and S t-shirt sizes are still good

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

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. ✨

@mdt2 mdt2 force-pushed the mdt2/css-611-migrate-action-group branch from 2869e63 to f55489b Compare January 22, 2024 16:36
@mdt2 mdt2 changed the title feat(actiongroup): migrate to S2 feat(actiongroup)!: migrate to S2 Jan 22, 2024
@pfulton pfulton force-pushed the mdt2/css-611-migrate-action-group branch from f55489b to 71d3327 Compare January 22, 2024 17:53
@pfulton pfulton closed this Jan 22, 2024
@pfulton pfulton reopened this Jan 22, 2024
@pfulton pfulton closed this Jan 22, 2024
@pfulton pfulton reopened this Jan 22, 2024
@pfulton pfulton force-pushed the mdt2/css-611-migrate-action-group branch from 71d3327 to 56d4e45 Compare January 22, 2024 20:17
Copy link
Contributor

github-actions bot commented Jan 22, 2024

File metrics

Summary

Total size: 3.89 MB*
Total change (Δ): ⬇ 23.71 KB (-0.59%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package Size Δ
actiongroup 5.70 KB ⬇ 2.80 KB
tokens 198.68 KB ⬇ 7.18 KB
Details

actiongroup

File Head Base Δ
index-base.css 5.70 KB 8.02 KB ⬇ 2.32 KB (-28.88%)
index-theme.css 0.59 KB 1.06 KB ⬇ 0.49 KB (-45.30%)
index-vars.css 5.70 KB 8.50 KB ⬇ 2.80 KB (-32.91%)
index.css 5.70 KB 8.50 KB ⬇ 2.80 KB (-32.91%)
mods.json 0.25 KB 0.40 KB ⬇ 0.15 KB (-37.47%)
themes/express.css 0.59 KB 0.89 KB ⬇ 0.30 KB (-33.33%)
themes/spectrum.css 0.59 KB 0.79 KB ⬇ 0.20 KB (-24.91%)

tokens

File Head Base Δ
css/dark-vars.css 36.30 KB 25.13 KB ⬆ 11.17 KB (44.43%)
css/express/custom-dark-vars.css 0.61 KB 0.61 KB -
css/express/custom-darkest-vars.css 0.61 KB 0.61 KB -
css/express/custom-large-vars.css 0.51 KB 0.51 KB -
css/express/custom-light-vars.css 0.65 KB 0.65 KB -
css/express/custom-medium-vars.css 0.49 KB 0.49 KB -
css/express/custom-vars.css 0.04 KB 0.04 KB -
css/global-vars.css 45.28 KB 39.57 KB ⬆ 5.71 KB (14.43%)
css/large-vars.css 27.90 KB 25.49 KB ⬆ 2.42 KB (9.48%)
css/light-vars.css 36.31 KB 25.13 KB ⬆ 11.18 KB (44.51%)
css/medium-vars.css 27.83 KB 25.42 KB ⬆ 2.41 KB (9.48%)
css/spectrum/custom-dark-vars.css 3.63 KB 3.63 KB -
css/spectrum/custom-darkest-vars.css 3.63 KB 3.63 KB -
css/spectrum/custom-large-vars.css 4.76 KB 4.76 KB -
css/spectrum/custom-light-vars.css 3.63 KB 3.63 KB -
css/spectrum/custom-medium-vars.css 5.00 KB 5.00 KB -
css/spectrum/custom-vars.css 2.10 KB 2.10 KB -
index.css 198.68 KB 205.86 KB ⬇ 7.18 KB (-3.49%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Jan 22, 2024

🚀 Deployed on https://pr-2453--spectrum-css.netlify.app

Copy link
Contributor

@jenndiaz jenndiaz left a 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.


/* 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));
Copy link
Contributor

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?

Copy link
Contributor

@jenndiaz jenndiaz Jan 26, 2024

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.

Copy link
Collaborator Author

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:

Screen Shot 2024-01-29 at 10 29 00 AM

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).

@mdt2 mdt2 force-pushed the mdt2/css-611-migrate-action-group branch from 56d4e45 to 1ff683e Compare January 29, 2024 15:46
@mdt2 mdt2 force-pushed the mdt2/css-611-migrate-action-group branch from 1ff683e to 8f59fe6 Compare January 29, 2024 15:58
Comment on lines -5 to -9
| `--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` |
Copy link
Collaborator Author

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?

Copy link
Collaborator

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!

@pfulton
Copy link
Collaborator

pfulton commented Jan 29, 2024

✅ approved by design via Slack.

@pfulton pfulton merged commit dbf9b9f into spectrum-two Feb 2, 2024
30 of 37 checks passed
@pfulton pfulton deleted the mdt2/css-611-migrate-action-group branch February 2, 2024 16:26
pfulton pushed a commit that referenced this pull request Feb 2, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Feb 5, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Feb 22, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Feb 26, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Mar 11, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Mar 19, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Mar 28, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Mar 28, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 1, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 1, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 3, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 4, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 5, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 11, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 11, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Apr 12, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Apr 15, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 18, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Apr 19, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request Apr 19, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 26, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 26, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 26, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 26, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 26, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
castastrophe pushed a commit that referenced this pull request Apr 30, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request May 1, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request May 3, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
rise-erpelding pushed a commit that referenced this pull request May 7, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
pfulton pushed a commit that referenced this pull request May 10, 2024
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants