-
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(ui-icons): migrate icon compiler to distinct package #2343
Conversation
File metricsSummaryTotal size: 4.03 MB*
Detailsui-icons
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
38fda1c
to
995bb3b
Compare
🚀 Deployed on https://pr-2343--spectrum-css.netlify.app |
995bb3b
to
568883c
Compare
c9944f0
to
776c5d5
Compare
358d92c
to
b18bbce
Compare
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.
I think this looks good. I looked through the code and checked a few components in the docs site and in Storybook and all the icons are matching prod. Should we run VRTs before merging?
533c41d
to
24b0db2
Compare
f1d6019
to
5df7824
Compare
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.
Just one thing here about the version number, @castastrophe
@@ -0,0 +1,28 @@ | |||
{ | |||
"name": "@spectrum-css/ui-icons", | |||
"version": "0.0.0", |
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.
Should we just make this 1.0.0?
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.
I did this because during the release it will bump it and I thought this would make it easier to target the 1.0 release we want. I'll change it during the release process if it doesn't bump correctly.
BREAKING CHANGE: - icon will no longer contain SVG assets
5df7824
to
c07e9e2
Compare
Description
Move SVG processing out of the CSS component package @spectrum-css/icon into a distinct package @spectrum-css/ui-icons.
Why?
Keeps tooling for SVG parsing separate from the tasks performed on CSS-only assets and allows consumers to pull in a focused package with the SVG assets they need for product.
How and where has this been tested?
Opened a PR against SWC with the necessary updates & tested this change using a yarn link to the new package.
Validation steps
To-do list