Skip to content

feat: simplify fallback of ui icon size numbers#3571

Merged
jawinn merged 11 commits into
spectrum-twofrom
jawinn/feat-s2-icons-remove-ui-fallback
Mar 4, 2025
Merged

feat: simplify fallback of ui icon size numbers#3571
jawinn merged 11 commits into
spectrum-twofrom
jawinn/feat-s2-icons-remove-ui-fallback

Conversation

@jawinn
Copy link
Copy Markdown
Collaborator

@jawinn jawinn commented Feb 20, 2025

Description

docs(icon): simplify fallback of ui icon size numbers

If a UI icon was not provided with its full name including sizing number (e.g. Dash100), the template would try and guess at a sizing number based on t-shirt size. This has been extracted out to a utility function and simplified, as this had gotten too complex, with too many exceptions. In some cases this was causing the wrong icon to be shown. This function no longer strips out any sizing numbers that were specifically provided.

docs(icon): log error when icon is not in the icon set

Update icon Storybook template with a guard clause so that it logs a warning when the icon is not in the icon set. Previously there was no indication except for not seeing the icon visually. This also renames the "idKey" variable to better explain what it is.

feat(icon): display placeholder icon when workflow icon is missing

Display the design recommended "Circle" icon as a placeholder icon when the workflow icon cannot be found:
Screenshot 2025-02-27 at 4 26 18 PM

This helps with identifying missing icons in VRTs, so the icon continues to take up the space it should, rather than disappearing entirely in some cases. This will help in the transition to the new S2 icons.


This change also updates the templates of a few components to make sure they are rendering the correct UI icon. This focused on components that use the icons whose sizing numbers are outside the normal / default mapping (arrow, asterisk, dash).

  • docs(button): replace deprecated ui icon size in test
  • feat(swatch): use spec defined ui icon size number for dash
  • fix(menu): update ui icon sizes used in template.
  • fix: icon set arg was missing a value in a few places
  • fix(actionbutton): define specific sizing for corner triangle

As part of this work, I manually ran a chromatic build on this branch and compared it against spectrum-two to assist with finding a few of these needed updates.

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

  • Icon stories and docs are still appearing correctly [@cdransf]
  • Button testing preview for large UI icon is now showing an icon [@cdransf]
  • UI icons are still appearing for the swatch dash, and menu chevron, menu checkmark, and menu (tray submenu) arrow [@cdransf]
  • Missing workflow icons, such as the ones in alert banner, are showing the placeholder circle icon [@cdransf]

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.
  • If my change impacts other components, I have tested to make sure they don't break.
  • ✨ This pull request is ready to merge. ✨

@jawinn jawinn added S2 Spectrum 2 wip This is a work in progress, don't judge. labels Feb 20, 2025
@jawinn jawinn changed the title feat(icon): remove fallback guesstimate of ui icon size numbers docs(icon): remove fallback guesstimate of ui icon size numbers Feb 20, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 20, 2025

File metrics

Summary

Total size: 1.38 MB*

🎉 No changes detected in any packages

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 20, 2025

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

@jawinn 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 Feb 20, 2025
@jawinn jawinn marked this pull request as ready for review February 21, 2025 13:57
@jawinn jawinn added the run_vrt For use on PRs looking to kick off VRT label Feb 21, 2025
@jawinn jawinn force-pushed the jawinn/feat-s2-icons-remove-ui-fallback branch from 31dd289 to ece7e0d Compare February 21, 2025 18:54
@jawinn jawinn changed the title docs(icon): remove fallback guesstimate of ui icon size numbers docs(icon): simplify fallback of ui icon size numbers Feb 21, 2025
@jawinn jawinn changed the title docs(icon): simplify fallback of ui icon size numbers feat: simplify fallback of ui icon size numbers Feb 21, 2025
@jawinn jawinn force-pushed the jawinn/feat-s2-icons-remove-ui-fallback branch from ece7e0d to fb0eeb6 Compare February 24, 2025 14:03
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Feb 24, 2025

⚠️ No Changeset found

Latest commit: d8a1ef7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pfulton pfulton 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 Feb 24, 2025
@castastrophe castastrophe force-pushed the spectrum-two branch 5 times, most recently from d5e72d4 to 0f14273 Compare February 24, 2025 22:39
@jawinn jawinn removed the run_vrt For use on PRs looking to kick off VRT label Feb 25, 2025
@jawinn jawinn force-pushed the jawinn/feat-s2-icons-remove-ui-fallback branch from fb0eeb6 to 7d26ca1 Compare February 25, 2025 19:42
@jawinn jawinn force-pushed the jawinn/feat-s2-icons-remove-ui-fallback branch 3 times, most recently from 7f7e721 to a7ad08f Compare February 27, 2025 21:56
@jawinn jawinn added ready-for-review and removed wip This is a work in progress, don't judge. labels Feb 27, 2025
@rise-erpelding rise-erpelding self-requested a review February 28, 2025 15:31
Copy link
Copy Markdown
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Thank you for all the work you do making icons better! Mostly I have some questions that I'm leaving in the comments that may or may not need addressing.

Comment thread components/actionbutton/stories/template.js Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I get some of these "Could not find SVG markup..." warnings on the menu Docs page for Checkmark and Chevron. Is that expected? Is it anything that needs to be addressed here?

image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I happened across those as well. They are related to the context needing to be passed through to the templates used in Menu (non-ref SVG markup comes from the global loader and its data is in context). I went ahead and just added a commit to fix that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I don't see a spot where we're failing to add context, but I'm still getting those warnings locally and in the preview for checkmark and chevron, which seems weird... Accordion uses chevron and it's not having that issue, so it feels like it's more specific to menu and less to icons, what do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I isolated it to just SubmenuInPopover. It was related to context not actually being passed in to the content array arg of Popover, because of how it was trying to do it as a function (eventually reaching renderContent in the Popover template which then doesn't pass in any args or context). I just updated the existing commit, so those particular warnings should be gone now on the Menu Docs page.

Comment thread components/icon/stories/template.js
Comment thread components/icon/stories/template.js
@jawinn jawinn requested a review from rise-erpelding March 3, 2025 15:04
Copy link
Copy Markdown
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Aside from menu still not being able to find the svg markup for chevron and checkmark, I think this is good to go--I'll leave it to you to decide whether this is something that can/needs to be addressed here or can be done separately.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I don't see a spot where we're failing to add context, but I'm still getting those warnings locally and in the preview for checkmark and chevron, which seems weird... Accordion uses chevron and it's not having that issue, so it feels like it's more specific to menu and less to icons, what do you think?

@jawinn jawinn force-pushed the jawinn/feat-s2-icons-remove-ui-fallback branch from 1887acd to a39c477 Compare March 3, 2025 21:57
Copy link
Copy Markdown
Member

@cdransf cdransf left a comment

Choose a reason for hiding this comment

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

Ran through the validation steps and everything looks good! ✨

jawinn added 11 commits March 4, 2025 12:13
If a UI icon was not provided with its full name including sizing number
(e.g. Dash100), the template would try and guess at a sizing number
based on t-shirt size. This has been extracted out to a utility
function and simplified, as this had gotten too complex, with too many
exceptions. It no longer strips out any sizing numbers that were
specifically provided.

docs(icon): log error when icon is not in the icon set

Update icon Storybook template with a guard clause so that it logs a
warning and returns when the icon is not in the icon set. Previously
there was no indication except for not seeing the icon visually.
Also renames "idKey" variable to better explain what it is.
- The Arrow icon is now only available in 100 and 400; adjusts which
  arrow number size is displayed for each component t-shirt size.
- The iconWithScale function was only the mapping for the arrow and
  was incorrectly used for the other icons like chevron and checkmark.
The previously referenced UI icon no longer exists in the UI icons
package. Replaces with the largest UI icon available.
Use the correct size number for the "Dash" ui icon per t-shirt size of
the swatch, according to its S2 design spec.
Make sure the iconset arg is set in a few components that are using
the Icon template, otherwise it was passed through as undefined. Also
set a default icon set in the templates used.
We have to define specific sizing for the UI icon corner triangle,
becauses there is no size 50 for extra small. Fixes XS action button
not showing the corner triangle icon.
Display the design recommended "Circle" icon as a placeholder icon
when the workflow icon cannot be found.

This helps with identifying missing icons in VRTs, so the icon
continues to take up the space it should, rather than disappearing
entirely in some cases.
Add context that was missing from some menu storybook templates, which
was causing a warning within Icon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants