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
Added Picker Network Component #16340
Conversation
Builds ready [03b3ef6]
Page Load Metrics (2297 ± 117 ms)
highlights: |
Builds ready [2847977]
Page Load Metrics (2307 ± 364 ms)
highlights: |
Builds ready [b6c4a2e]
Page Load Metrics (2318 ± 122 ms)
highlights: |
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.
Really great start! Have left some suggestions
ui/components/component-library/picker-network/picker-network.js
Outdated
Show resolved
Hide resolved
ui/components/component-library/picker-network/picker-network.scss
Outdated
Show resolved
Hide resolved
ui/components/component-library/picker-network/picker-network.stories.js
Outdated
Show resolved
Hide resolved
Builds ready [994de6c]
Page Load Metrics (2334 ± 118 ms)
highlights: |
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.
Looking good! Couple more suggestion that I missed in my previous review. My bad!
-
Could we add a story for
label
prop and show it under the Label section in the MDX docs. The story could include a really long name? Should we truncate if the network name is super long? I've asked @Akatori-Design -
Could we add a story for the
src
prop and show it under the Src section in MDX docs. This story could show a few PickerNetwork components with different network images -
Add a snapshot test. Checkout @garrettbear as an example
-
Checked storybook controls ✅
-
Checked no errors in console ✅
-
Checked MDX docs ✅
-
Checked house-keeping checks 🛑 added comments for missing criteria above
Builds ready [cfb9b3e]
Page Load Metrics (2094 ± 87 ms)
highlights:storybook
|
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.
LGTM! One suggestion non-blocking
Builds ready [431b3d7]
Page Load Metrics (2378 ± 57 ms)
highlights:storybook
|
Builds ready [44dbefb]
Page Load Metrics (2097 ± 96 ms)
highlights:storybook
|
After
Manual Testing Steps
Storybook
Pre-Merge Checklist
+ If there are functional changes: