refactor(icon)!: tokens migration #2347

jawinn · 2023-12-06T22:05:17Z

Description

Migration of the icon component from DNA tokens to Spectrum tokens.

Icon sizing is now handled with custom properties. The local variables --spectrum-icon-inline-size and --spectrum-icon-block-size represent the size of the icon. New --mod properties are added for adjusting the size of the icon, if needed:

--mod-icon-size -- can be used to adjust both inline-size and block-size with one value, rather than individually
--mod-icon-inline-size -- customize this dimension; will take precedence over --mod-icon-size
--mod-icon-block-size -- customize this dimension; will take precedence over --mod-icon-size

This also simplifies and clarifies how the "combined" medium/large UI icons are used and removes a legacy fallback. The "combined" icons include both medium and large scale icons within one file and show/hide them based on the current scale. The following mods are provided if the display property of these ever needs to be adjusted:

--mod-ui-icon-medium-display
--mod-ui-icon-large-display

The following updates and bugfixes have also been made as part of the migration:

Removes the postcss-custom-properties-passthrough plugin from the build which is no longer necessary after refactoring away the "xvar" code.
Add XXS workflow icon size to support existing size in SWC. Note that both XXS and XXL are not in the design spec and are planned to be deprecated in Spectrum 2.
Gripper icon classes removed: The gripper icon classes were named incorrectly (e.g. .spectrum-UIIcon-TripleGripper100 and not used, and there are no tokens defined yet to set the actual classes (e.g. .spectrum-UIIcon-TripleGripper) to. They were using '100' size naming in the classes, which are not currently used or displayed. There were other issues with the previous intended values as well; see relevant commit description for more details.
Adds Chromatic only kitchen sink style story template, so various icon sets and sizes are covered in VRTs. See screenshot below.
Updates UI Icon sizing options in Storybook to not use t-shirt sizing, and to include number sizing (see "UI Icon sizing in Storybook" below for details).
Fixes a Storybook bug where workflow arrows and chevrons were incorrectly rendering as a different UI icon with the same name.

Some discoveries and topics for additional discussion:

spectrum-UIIcon:
The class .spectrum-UIIcon does not appear to be used within the repo. Looking at the CSS, it seems like the original intention was to have either .spectrum-UIIcon or .spectrum-Icon used, but currently all of our uses of UI icons use .spectrum-Icon which has default workflow sizing. This historically would not cause any sizing differences, since defined UI icons set their own specific sizes that override the workflow sizing, and the rest of the CSS between the two classes is shared. I'm unclear as to exactly how this was intended or if any future changes are warranted there.

UI Icon sizing in Storybook:
UI icons do not use t-shirt sizing like workflow icons do, but Storybook used it as a control. It had a switch statement that provides some sizing numbers mapped to t-shirt sizes, but this did not account for all of the UI icon size numbers. UI Icons have specific sizes with number sizing, and some UI icons support number sizes that other UI icons do not (e.g. a *-50 or a *-500). I've made an update to list every UI icon with its size on the icon name select menu. Ideally it'd be nice to have a separate size number control with available size numbers conditionally showing for each UI icon name--currently there are no conditional options feature available in Storybook to provide each specific list of size numbers for each UI icon in Storybook.

CSS-511

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 @jenndiaz

Confirm that workflow and UI icons are appearing in Storybook
The Storybook "size" control continues to work the same for workflow icons
Icons are still appearing in other stories that import the Icon template
Icons are still appearing on docs site components that use icons
XXL icon sizing still works (top of docs site pages' icon links and Card component "Horizontal" variant)
Chromatic-only kitchen sink style template is appearing in Chromatic
All UI Icon number sizes appear in Storybook when selecting icon name, and workflow sizing control no longer appears for UI icon set
Confirm bugfix: correct arrows and chevrons are now rendering for the workflow icon set; workflow arrows should appear as a different, much wider icon than the UI icon set arrows.

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

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.

Screenshots

Icon kitchen sink for Chromatic, showing examples of all sizes for a few Workflow icons, and every available size number for all UI icons:

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

github-actions · 2023-12-19T22:01:21Z

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

github-actions · 2023-12-19T22:02:33Z

File metrics

Summary

Total size: 3.94 MB^*
Total change (Δ): ⬆ 18.79 KB (0.47%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package	Size	Δ
icon	11.42 KB	⬆ 11.42 KB

Details

icon

File	Head	Base	Δ
index-base.css	new	-	⬆ 11.42 KB (Infinity%)
index-vars.css	11.42 KB	12.21 KB	⬇ 0.80 KB (-6.42%)
index.css	new	-	⬆ 11.42 KB (Infinity%)
mods.json	0.20 KB	0.04 KB	⬆ 0.16 KB (460.00%)

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

pfulton

Looks like the Table with ActionBar variant might be displaying incorrectly on the docs site: https://pr-2347--spectrum-css.netlify.app/actionbar#tablewithactionbar

This was the only problem that I noticed. The code here looks good!

jawinn · 2024-01-11T21:01:28Z

Looks like the Table with ActionBar variant might be displaying incorrectly on the docs site: https://pr-2347--spectrum-css.netlify.app/actionbar#tablewithactionbar

This was the only problem that I noticed. The code here looks good!

This issue is in main as well and looks to be build related. It happens when I run main right now locally--it's not showing on the prod docs probably because they have not rebuilt recently. This docs page is missing the inclusion of components/checkbox/index-vars.css, so the checkbox doesn't have its styles. Does this docs page need an added dependency for table? @castastrophe It's only displayed in the examples but is not really a part of ActionBar.

components/icon/stories/icon.stories.js

castastrophe

A suggestion on the Chromatic snapshot format but otherwise this looks perfect. Can you rebase against main when you get a chance?

pfulton

Looks great!

pfulton · 2024-01-23T22:05:40Z

Beta version released: @spectrum-css/icon@6.0.0-beta.0

pfulton · 2024-02-01T21:17:55Z

Published two new beta versions:

 - @spectrum-css/icon@6.0.0-beta.1
 - @spectrum-css/tokens@13.1.0-beta.1

components/icon/workflow-icons.css

BREAKING CHANGE: Build system changes to migrate from DNA tokens to Spectrum tokens.

Migrates Icon CSS from using DNA/vars tokens to Spectrum tokens. Refactors UI Icons to be a little cleaner and not need placeholders. Icons now change the value of the property "--spectrum-icon-size" to set their width and height. They also have three additional mods available for setting the size (both width and height) or the individual width and height.

- Remove 'xvar' and 'x--' code within UI icon CSS, along with the build plugins that were used only for this. This was only only needed previously when the build did not allow 'var()' and '--property' here. - Simplify and better document code used for combined UI icons and the medium/large platform scale. Remove old browser support here that is no longer needed with the browsers and features that are currently supported by the library. The old fallback to set display inline was pre Firefox version 56 [2017].

Removing the gripper icon classes as they were incorrect and not used, and there are no tokens defined yet to set the actual classes to. The gripper icon classes used previously were wrong in several ways. For one, they were using '100' size naming in the classes which are not currently used or displayed. These icons are without the number size. The old alias values being applied to them also looked incorrect when looking at the widths, and the CSS was swapping width for height. That there is no size applied to these icons was obfuscated by the fact that the attribute width="10" is being applied to icons in Storybook. Note: SWC is currently showing these icons with workflow sizing. These gripper icons do not have size tokens defined yet, but they may be added in the future "as they are needed"; when these icons start being used.

Cover the various types of icons in a Chromatic only story. Covers different icon sets, sizes, and color in the VRTs.

Make sure we always have custom properties that contains the width and height, that we can rely upon for CSS calculations. Regardless of whether the individual dimensions are specified or just the size is specified (that applies to both dimensions).

The icons in Storybook were adding an inline "width" attribute set to 10, which was previously obfuscating issues with sizing. Removes this attribute and leaves sizing up to CSS.

Added extra small workflow icon size. This has a token, is defined on some of the design redlines (Action Button), and is currently used in the Contextual Help component, as seen in the VRT run.

Recent updates to main make it no longer necessary to include empty theme files for the build to work.

Disables the size control for UI icons and adds each size number to the list of available UI icon names in Storybook. UI icons have specific sizing and don't use the t-shirt sizing that Workflow icons do. They have more size numbers than there are t-shirt sizes, so they can't be directly mapped to each other. The different UI icons have different size numbers, so the size numbers can't use a single control.

Show all UI icons, including number sizing, in the Chromatic template. Condenses and improves some of the template logic.

Fix bug where the wrong icon was being rendered for workflow arrow and chevron. These are both icons with names that exist in both icon sets. There was logic being applied to the workflow icons that should have only have been applied to the UI icon.

Add XXS size to support existing SWC size. Uses the values from --spectrum-global-dimension-size-150, as used in SWC's custom icon CSS. Included comments to note that xxs and xxl are planned to be deprecated in Spectrum 2, as they are not a part of the design spec.

Update required tokens version with a minimum of the latest release that includes the new custom-vars for the xxl and xxs workflow icon sizes.

castastrophe

Amazing! Thank you!

jawinn marked this pull request as draft December 6, 2023 22:14

jawinn force-pushed the jawinn/css-511-icon-migration branch from 35337c6 to 07752d3 Compare December 19, 2023 21:53

jawinn force-pushed the jawinn/css-511-icon-migration branch from 07752d3 to ff9d941 Compare December 19, 2023 22:11

jawinn marked this pull request as ready for review December 19, 2023 22:15

jawinn force-pushed the jawinn/css-511-icon-migration branch 2 times, most recently from 4c102fb to e28f922 Compare January 2, 2024 23:13

jawinn added the run_vrt For use on PRs looking to kick off VRT label Jan 3, 2024

pfulton self-requested a review January 8, 2024 20:30

pfulton requested changes Jan 8, 2024

View reviewed changes

jawinn force-pushed the jawinn/css-511-icon-migration branch 2 times, most recently from afa629c to cef588d Compare January 11, 2024 20:24

castastrophe reviewed Jan 17, 2024

View reviewed changes

components/icon/stories/icon.stories.js Outdated Show resolved Hide resolved

castastrophe requested changes Jan 17, 2024

View reviewed changes

jawinn force-pushed the jawinn/css-511-icon-migration branch 2 times, most recently from 81bf501 to 53d45bb Compare January 22, 2024 19:24

jawinn added run_vrt For use on PRs looking to kick off VRT and removed run_vrt For use on PRs looking to kick off VRT labels Jan 22, 2024

jawinn force-pushed the jawinn/css-511-icon-migration branch 3 times, most recently from 27212df to ba6b202 Compare January 23, 2024 15:15

castastrophe force-pushed the main branch from be2974e to ff5dda6 Compare January 23, 2024 15:54

jawinn force-pushed the jawinn/css-511-icon-migration branch from ba6b202 to de6ad34 Compare January 23, 2024 16:34

jawinn requested review from pfulton, castastrophe and jenndiaz January 23, 2024 19:18

pfulton approved these changes Jan 23, 2024

View reviewed changes

jawinn force-pushed the jawinn/css-511-icon-migration branch from 556d411 to 34ef0be Compare February 1, 2024 21:07

Westbrook reviewed Feb 1, 2024

View reviewed changes

components/icon/workflow-icons.css Outdated Show resolved Hide resolved

jawinn force-pushed the jawinn/css-511-icon-migration branch from 5c8b6d8 to 5e8803f Compare February 2, 2024 17:16

pfulton force-pushed the jawinn/css-511-icon-migration branch from db0fd19 to ec056b4 Compare February 2, 2024 20:54

jawinn mentioned this pull request Feb 2, 2024

feat(tokens): add icon xxl and xxs tokens #2485

Merged

5 tasks

jawinn force-pushed the jawinn/css-511-icon-migration branch 2 times, most recently from a711325 to 165f3c0 Compare February 5, 2024 16:04

pfulton approved these changes Feb 5, 2024

View reviewed changes

jawinn mentioned this pull request Feb 5, 2024

docs: storybook updates for icon sets, arrow and chevron #2491

Merged

6 tasks

jawinn and others added 16 commits February 5, 2024 13:59

build(icon)!: migrate icon build to spectrum tokens

6c2c000

BREAKING CHANGE: Build system changes to migrate from DNA tokens to Spectrum tokens.

docs(icon): storybook - add kitchen sink style story for chromatic

31525f8

Cover the various types of icons in a Chromatic only story. Covers different icon sets, sizes, and color in the VRTs.

fix(icon): storybook - remove inline width attribute

e4bc40a

The icons in Storybook were adding an inline "width" attribute set to 10, which was previously obfuscating issues with sizing. Removes this attribute and leaves sizing up to CSS.

feat(icon): support for xs workflow icon size

9239f12

Added extra small workflow icon size. This has a token, is defined on some of the design redlines (Action Button), and is currently used in the Contextual Help component, as seen in the VRT run.

feat(icon): remove theme files without content

f098a69

Recent updates to main make it no longer necessary to include empty theme files for the build to work.

feat(icon): storybook - show all ui icons in chromatic template

34b8659

Show all UI icons, including number sizing, in the Chromatic template. Condenses and improves some of the template logic.

chore(icon): manual version bump for beta release

06da27e

chore(icon): set current beta versions already released

978bfd8

build(icon): minimum tokens version with xxl and xxs sizing

4f6537e

Update required tokens version with a minimum of the latest release that includes the new custom-vars for the xxl and xxs workflow icon sizes.

jawinn force-pushed the jawinn/css-511-icon-migration branch from 165f3c0 to 4f6537e Compare February 5, 2024 19:00

castastrophe approved these changes Feb 5, 2024

View reviewed changes

pfulton merged commit b63631a into main Feb 5, 2024

pfulton deleted the jawinn/css-511-icon-migration branch February 5, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(icon)!: tokens migration #2347

refactor(icon)!: tokens migration #2347

Uh oh!

jawinn commented Dec 6, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 19, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 19, 2023 •

edited

Loading

icon

Uh oh!

pfulton left a comment

Uh oh!

jawinn commented Jan 11, 2024

Uh oh!

Uh oh!

castastrophe left a comment

Uh oh!

pfulton left a comment

Uh oh!

pfulton commented Jan 23, 2024

Uh oh!

pfulton commented Feb 1, 2024

Uh oh!

Uh oh!

castastrophe left a comment

Uh oh!

Uh oh!

refactor(icon)!: tokens migration #2347

refactor(icon)!: tokens migration #2347

Uh oh!

Conversation

jawinn commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How and where has this been tested?

Validation steps @jenndiaz

Regression testing

Screenshots

To-do list

Uh oh!

github-actions bot commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

icon

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

jawinn commented Jan 11, 2024

Uh oh!

Uh oh!

castastrophe left a comment

Choose a reason for hiding this comment

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

pfulton commented Jan 23, 2024

Uh oh!

pfulton commented Feb 1, 2024

Uh oh!

Uh oh!

castastrophe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jawinn commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Dec 19, 2023 •

edited

Loading