This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add storybook story for icon library #2787
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This also involved modifying the dashicon module replacement to use `cloneElement` instead of `createElement`.
The Woo category icon was not using the Icon system and not exported consistent with other icons.
nerrad
added
tools
|
Size Change: +819 B (0%) Total Size: 1.59 MB
ℹ️ View Unchanged
|
Aljullu
approved these changes
Jun 26, 2020
There was a problem hiding this comment.
Everything is testing well and code looks good.
I discovered while testing, that although the grid blocks are initializing with the expected defaults, all attributes are not getting saved with the blocks as expected.
Should we open an issue to investigate that?
| /> | ||
| <div | ||
| style={ { | ||
| display: 'flex', |
There was a problem hiding this comment.
What's the benefit of having inline styles instead of a style.scss file? The QuantitySelector component has a stories/style.scss file, so it might be good to decide what's the best approach and be consistent with it.
There was a problem hiding this comment.
This is just a story and not a component we use so I'm just using inline styles for the purpose of the story :)
There was a problem hiding this comment.
The QuantitySelector component has a stories/style.scss file, so it might be good to decide what's the best approach and be consistent with it.
In that case the css class is being modified for the selector on the component. Personally I think we should avoid styles on actual components as much as possible in stories (favoring what the default look for the component is). That will help us catch potential issues with the default not being usable.
Co-authored-by: Albert Juhé Lluveras <[email protected]>
Yup, see #2789. |
6 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull adds a storybook story for the
@woocommerce/iconslibrary.This is pretty much a direct take from the equivalent Gutenberg implementation except I modified the styling a bit so more icons display on the screen. The story is setup so that any new icons added to the library will automatically display with the story.
In the process of doing this pull the story revealed the following:
wpglobal for filters we were using instead of importing from the package (external)cloneElementinstead ofcreateElement.width,heightand other props (so cloning to pass those values through would work). I updated the implementation where the category is registered so that it's implemented using theIconcomponent pattern used for all icons in the library.arrowBackicon from the library didn't haveviewBoxset, so that meant it wasn't reflecting changes in size properly.To Test
The story can be tested by running
npm run storybookand viewing the story for the icons. When this pull is merged to master they should show up in the generated storybook as well.Due to the changes in other files, there should be some smoke testing of the impacted code:
wp global to package import
So the interesting thing here is that this code is related to the work don in this pull. However, I discovered while testing, that although the grid blocks are initializing with the expected defaults, all attributes are not getting saved with the blocks as expected. This is also happening on master too. So I'm not sure if there was another change that impacted this (maybe filters loading too late?) but if this behaviour is unexpected we'll likely need to investigate via a follow-up issue (cc @mikejolley).
Dashicon replacements.
The affected icons are the chevron on our selects (in checkout):
...and the "x" dismiss icon in notifications:
Woo Icon
Verify that the woo icon displays correctly for the WooCommerce Blocks category in the Block picker.
arrowBackicon.This icon is used with the "Return to Cart" item in the checkout block:
arrowBackicon displays as expected.