WB-1611: Add support to custom paths/SVGs with PhosphorIcon #2117
Conversation
🦋 Changeset detectedLatest commit: 2e33cfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Size Change: +283 B (0%) Total Size: 92.9 kB
ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2117 +/- ##
==========================================
- Coverage 97.09% 97.06% -0.04%
==========================================
Files 243 243
Lines 28205 28233 +28
Branches 2463 2422 -41
==========================================
+ Hits 27386 27404 +18
- Misses 819 829 +10
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-umycqcuufe.chromatic.com/ Chromatic results:
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (c9772d6) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2117"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2117 |
| */ | ||
| icon: PhosphorIconMedium; | ||
| icon: PhosphorIconAsset | string; |
There was a problem hiding this comment.
Suggestion: Can you update the argtypes to reflect this?
I also think it might be nice to have a story demonstrating how to use a PhosphorIconAsset vs string. I see you have a string example in the WithColor story, but I think it might be helpful to have something more explicit somewhere. And maybe you can also mention if one approach is preferred over the other if that's the case?
There was a problem hiding this comment.
The string case is covered in the CustomStyles story. I'll update the args table to be more explicit about that.
Co-authored-by: Nisha Yerunkar <[email protected]>
There was a problem hiding this comment.
LGTM
My review uses Conventional Comments to add context and set expectations for the comments I am leaving.
| */ | ||
| // export const CustomIcon: StoryComponentType = () => { |
There was a problem hiding this comment.
praise: Thanks for tidying up this.
Summary:
Removed the limitation of only using certain icon weights per size. Now we can
use any icon weight with any size. Reworded the documentation to still recommend
(but not enforce) those rules.
Added support for custom SVG imports. This is useful for icons that are not
available in the PhosphorIcon library. This change was done by allowing more SVG
paths in TypeScript. The
PhosphorIconcomponent now accepts any SVG import asa prop.
Issue: https://khanacademy.atlassian.net/browse/WB-1611
Test plan:
Verify that the "Custom icons" story displays all the custom icons correctly.
?path=/docs/icon-phosphoricon--docs#custom-icons