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.

Sign up for GitHub

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

Jump to bottom

refactor(ui-icons)!: s2 migration of assets #2657

Closed
wants to merge 40 commits into from
Closed

refactor(ui-icons)!: s2 migration of assets #2657

wants to merge 40 commits into from

Conversation

castastrophe
Copy link
Collaborator

@castastrophe castastrophe commented Apr 11, 2024 •
edited

Description

This PR pulls in the S2 icons and commits the dist output. This is a breaking change with entirely new icons.

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:

  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.

Screenshots

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 GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 11, 2024 •
edited

File metrics

Summary

Total size: 4.48 MB*

🎉 No changes detected in any packages

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

@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 3 times, most recently from a059899 to 1057857 Compare April 11, 2024 16:28
@castastrophe castastrophe added the S2 Spectrum 2 label Apr 11, 2024
@castastrophe castastrophe self-assigned this Apr 11, 2024
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch from 1057857 to d1a56a3 Compare April 11, 2024 16:35
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 3 times, most recently from 6edf279 to a92428f Compare April 11, 2024 20:19
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 11, 2024 •
edited

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

@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 2 times, most recently from 9632c6e to c0c15ea Compare April 11, 2024 20:58
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 3 times, most recently from 04749d5 to 9194190 Compare April 15, 2024 17:32
@castastrophe castastrophe marked this pull request as ready for review April 15, 2024 17:37
@castastrophe castastrophe added run_vrt For use on PRs looking to kick off VRT ready-for-review labels Apr 15, 2024
@castastrophe castastrophe changed the title refactor(ui-icons): s2 migration of assets refactor(ui-icons)!: s2 migration of assets Apr 15, 2024
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch from 9194190 to 5742bf2 Compare April 15, 2024 20:06
@jawinn jawinn self-requested a review April 16, 2024 14:08
jawinn
jawinn requested changes Apr 16, 2024
View reviewed changes
Copy link
Collaborator

@jawinn jawinn left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version of these icons is quite a big change! I have a few questions and noticed a few issues.

Missing Workflow icons

Some workflow icons (Chevron, Arrow) are no longer displaying. I believe I see what was causing this, as noted in the code comments.

Order of UI icons displayed

It would be good if the arrows showed up next to each other here. They are spread out in different areas of the list. This also happens on the select menu for the icon in the Storybook control. I would expect all of the icon names starting with "Arrow" to be next to each other:
Screenshot 2024-04-16 at 10 35 39 AM

Needed CSS changes and refactoring: classes with sizing for new UI icons, and removal of Medium/Large related code

Each UI icon is paired with a class and token that determines its size. With the addition of new icons and removal of some icons, there is now CSS that needs to be both added and removed. This can be seen currently in the first set of five "Add" icons all appearing as the same size, since there is no .spectrum-UIIcon-Add50 etc:
Screenshot 2024-04-16 at 10 43 21 AM
It doesn't look like some of these new icons have tokens that exist yet.

The removal of the separate Large and Medium UI icons also means that some code can be refactored and removed.

This could be follow-up work. Should we create another Jira issue for this?

Communication of deprecations?

This update removes a lot of existing sizes and icons. For example, Arrow200, Arrow300, Asterisk75, etc. Should we communicate these changes somehow? Those sizes are likely in use for some users of the library. Some are also in use in our own repo, though I am assuming once everything is migrated to Spectrum 2 then they won't be anymore.

Storybook Chromatic-only template reference

On PR
Screenshot 2024-04-16 at 10 04 19 AM

Production
Screenshot 2024-04-16 at 10 05 13 AM

components/icon/stories/template.js Outdated Show resolved Hide resolved
components/icon/stories/template.js Outdated Show resolved Hide resolved
components/icon/stories/utilities.js Outdated Show resolved Hide resolved
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 2 times, most recently from 7befcb2 to 009ab72 Compare April 18, 2024 19:46
pfulton and others added 9 commits April 26, 2024 16:37
@pfulton @castastrophe
chore(release): release
48f6b2f 
 - @spectrum-css/preview@8.1.0-next.1
 - @spectrum-css/accordion@4.2.7-next.2
 - @spectrum-css/actionbar@7.2.5-next.2
 - @spectrum-css/actionbutton@5.2.7-next.2
 - @spectrum-css/actiongroup@5.0.0-next.3
 - @spectrum-css/actionmenu@5.1.4-next.2
 - @spectrum-css/alertbanner@1.1.42-next.2
 - @spectrum-css/alertdialog@1.2.5-next.2
 - @spectrum-css/asset@4.0.1-next.2
 - @spectrum-css/assetcard@3.1.5-next.2
 - @spectrum-css/assetlist@5.2.5-next.2
 - @spectrum-css/avatar@6.1.6-next.2
 - @spectrum-css/badge@3.2.6-next.2
 - @spectrum-css/breadcrumb@8.2.6-next.2
 - @spectrum-css/button@14.0.0-next.4
 - @spectrum-css/buttongroup@7.0.0-next.3
 - @spectrum-css/calendar@4.2.6-next.2
 - @spectrum-css/card@7.0.3-next.2
 - @spectrum-css/checkbox@14.0.0-next.4
 - @spectrum-css/clearbutton@5.1.5-next.2
 - @spectrum-css/closebutton@5.0.0-next.2
 - @spectrum-css/coachindicator@1.1.6-next.2
 - @spectrum-css/coachmark@6.1.6-next.2
 - @spectrum-css/colorarea@4.1.6-next.2
 - @spectrum-css/colorhandle@7.1.5-next.2
 - @spectrum-css/colorloupe@4.2.5-next.2
 - @spectrum-css/colorslider@5.1.6-next.2
 - @spectrum-css/colorwheel@3.1.6-next.2
 - @spectrum-css/combobox@2.1.7-next.2
 - @spectrum-css/commons@9.1.4-next.1
 - @spectrum-css/contextualhelp@2.1.6-next.2
 - @spectrum-css/datepicker@2.1.5-next.2
 - @spectrum-css/dial@2.2.5-next.2
 - @spectrum-css/dialog@9.2.5-next.2
 - @spectrum-css/divider@2.2.6-next.2
 - @spectrum-css/dropindicator@4.1.5-next.2
 - @spectrum-css/dropzone@5.2.6-next.2
 - @spectrum-css/fieldgroup@4.2.5-next.2
 - @spectrum-css/fieldlabel@8.0.0-next.2
 - @spectrum-css/floatingactionbutton@1.2.6-next.2
 - @spectrum-css/helptext@4.1.6-next.2
 - @spectrum-css/icon@6.0.6-next.2
 - @spectrum-css/illustratedmessage@6.1.6-next.2
 - @spectrum-css/infieldbutton@4.2.5-next.2
 - @spectrum-css/inlinealert@7.1.7-next.2
 - @spectrum-css/link@4.2.6-next.2
 - @spectrum-css/logicbutton@3.3.5-next.2
 - @spectrum-css/menu@6.1.6-next.2
 - @spectrum-css/miller@5.1.5-next.2
 - @spectrum-css/modal@4.2.7-next.2
 - @spectrum-css/opacitycheckerboard@1.1.6-next.2
 - @spectrum-css/page@7.1.5-next.2
 - @spectrum-css/pagination@7.1.7-next.2
 - @spectrum-css/picker@7.2.8-next.2
 - @spectrum-css/pickerbutton@4.1.6-next.2
 - @spectrum-css/popover@6.2.6-next.2
 - @spectrum-css/progressbar@3.1.6-next.2
 - @spectrum-css/progresscircle@2.2.4-next.2
 - @spectrum-css/radio@8.1.6-next.2
 - @spectrum-css/rating@4.2.5-next.2
 - @spectrum-css/search@6.2.5-next.2
 - @spectrum-css/sidenav@4.2.5-next.2
 - @spectrum-css/site@4.2.5-next.2
 - @spectrum-css/slider@4.3.6-next.2
 - @spectrum-css/splitview@4.2.5-next.2
 - @spectrum-css/statuslight@6.1.7-next.2
 - @spectrum-css/steplist@4.1.5-next.2
 - @spectrum-css/stepper@5.1.6-next.2
 - @spectrum-css/swatch@5.1.6-next.2
 - @spectrum-css/swatchgroup@2.1.6-next.2
 - @spectrum-css/switch@4.2.6-next.2
 - @spectrum-css/table@5.2.6-next.2
 - @spectrum-css/tabs@4.1.5-next.2
 - @spectrum-css/tag@8.1.6-next.2
 - @spectrum-css/taggroup@4.1.6-next.2
 - @spectrum-css/textfield@6.1.7-next.2
 - @spectrum-css/thumbnail@5.2.5-next.2
 - @spectrum-css/toast@9.1.26-next.2
 - @spectrum-css/tooltip@5.3.6-next.2
 - @spectrum-css/tray@2.2.8-next.2
 - @spectrum-css/treeview@9.2.7-next.2
 - @spectrum-css/typography@5.1.6-next.2
 - @spectrum-css/underlay@3.2.5-next.2
 - @spectrum-css/well@4.1.5-next.2
 - @spectrum-css/generator@2.1.1-next.0
 - @spectrum-css/tokens@14.0.0-next.6
@mdt2 @castastrophe
feat(switch): s2 migration (#2651)
16dc834 
* feat(switch): s2 migration

* chore: remove themes

* fix: animation

* fix: whcm light mode unchecked handle shows

* docs(storybook): align chromatic setup with new standard

* fix: checked corners for non-retina display

* fix: whcm handle selected color

* fix: namespace mods, use semantic tokens instead of global

* fix: handle state colors
@pfulton @castastrophe
chore: prerelease mode for changesets
8db2fae
@pfulton @castastrophe
chore(tokens): use @adobe/spectrum-tokens@13.0.0-beta.30 (#2678)
7aff2e6 
* chore(tokens): use spectrum-tokens@13.0.0-beta.30
* chore: add changeset
@pfulton @castastrophe
chore: remove darkest as SB option (#2679)
d64d450
@github-actions @pfulton @castastrophe
chore: release (next) (#2680)
44e2291 
* chore: release (next)

* docs: reset changes to changelog

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Patrick Fulton <pfulton@adobe.com>
@castastrophe @jawinn @pfulton
chore: run branch through format; update pathing and fix errors (#2700)
5f53aa9 
* feat(fieldlabel)!: s2 migration (#2569)

BREAKING CHANGE: uses Spectrum 2 tokens

Also:
* feat(fieldlabel): medium as default and 100 sizing tokens

- Use the correct "100" sized tokens for medium.
- Make medium styles the default, to conform with existing pattern.
  .spectrum-FieldLabel--sizeM class removed as it is no longer
  necessary.

* feat(fieldlabel): use correct font size tokens

The font-size tokens used for large and extra large needed to be shifted
up to match the s2 spec.

* refactor(fieldlabel): remove deprecated mods

Remove renamed mods marked as deprecated for S2 release.

* docs(fieldlabel): spectrum 2 noted in migration notes

Updates some docs and adds migration notes. Clarifies some docs around
"left" and "right", and fixes duplicate ID warnings from examples code.

* docs(fieldlabel): form - docs update and regenerate mods

Add migration guide section to "Form", that currently lives within the
Field label component. Regenerates mods to include the removal of
deprecated renamed mods.

* fix(fieldlabel): rename mod property referencing global token

A mod custom property was referencing a global token instead of the
component itself. Renames this and adds a note to the migration guide.

* feat(fieldlabel): use correct sizing tokens

Use correct tokens for some large and extra large custom properties,
to matching with design.

* style(fieldlabel): applying linter formatting adjustments

Run linter fix on the fieldlabel component, which rearranges a few
declarations.

* docs(fieldlabel): document asterisk icon size

Add documentation about asterisk UI icon sizes specified on the design.
These sizes are already handled in the Storybook template, but were not
really explained to the user.

* feat(fieldlabel): black and white static color variants

Add classes for static colors, as defined on the designs. And adds
examples of these variants to the documentation.

* build(fieldlabel): specify s2 tokens release for dependency

Use Spectrum 2 version of tokens package in dependencies list.

* chore: rebase against main and update components to use prerelease tokens

* chore: run branch through format; update pathing and fix errors

* chore: add changeset for css update

---------

Co-authored-by: Josh Winn <965114+jawinn@users.noreply.github.com>
Co-authored-by: Patrick Fulton <pfulton@adobe.com>
@github-actions @castastrophe
chore: release (next) (#2706)
2b6c140 
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@castastrophe
chore: fix linting conflict vs. format
ae7aedd
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 7 times, most recently from 17809fd to 28d8639 Compare April 26, 2024 22:43
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 2 times, most recently from 489f2d8 to f2fc120 Compare April 26, 2024 23:00
@jawinn
Copy link
Collaborator

jawinn commented Apr 29, 2024

@jawinn I tried to adjust for the issues you raised but there were some significant changes to the format and naming of the icons so I tried to approach it in a new way. You can see the new baseline proposed here - let me know what you think! https://pr-2657--spectrum-css.netlify.app/preview/?path=/story/components-icon--default&globals=testingPreview:!true

The new grid layout is great. It's much more effective at showing which size is which. Could you also include an arrow and a chevron in the list of workflow icons? That way we can continue to test for any conflicts with those icons that can have the same name as UI icons (which had rendering issues previously).

The ordering of the icon names in the control looks like it still needs an update so they are alphabetical (e.g. Arrow* names grouped with each other).

What do you think about the questions raised in "Needed CSS changes and refactoring: classes with sizing for new UI icons, and removal of Medium/Large related code"? As well as communicating the deprecations?

@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch 5 times, most recently from 3c41671 to 144f109 Compare April 30, 2024 16:35
@castastrophe castastrophe force-pushed the refactor-ui-icons-s2-migration branch from 144f109 to 9c8bad0 Compare April 30, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfulton pfulton Awaiting requested review from pfulton

@jawinn jawinn Awaiting requested review from jawinn

Assignees

@castastrophe castastrophe

Labels
ready-for-review run_vrt For use on PRs looking to kick off VRT S2 Spectrum 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@castastrophe @jawinn @pfulton @mdt2 @jenndiaz