UI: Update every icon for v7 design #18809
Conversation
| ); | ||
| }; | ||
|
|
||
| export type IconType = keyof typeof icons; | ||
|
|
||
| export interface SymbolsProps extends ComponentProps<typeof Svg> { | ||
| icons?: IconKey[]; | ||
| } | ||
|
|
||
| export const Symbols = memo<SymbolsProps>(({ icons: keys = Object.keys(icons) }) => ( |
There was a problem hiding this comment.
This component differed from the one in the Design System in that there was an effort to make a symbol instead of using SVGs directly (sort of like a sprite). I left that functionality in tact here, but I am unsure if it is necessary. Thoughts?
MichaelArestad
force-pushed
the
update-every-icon
branch
from
c50eff0
to
d33222e
Compare
…size, and resolved ts error
There was a problem hiding this comment.
The code and visual output here looks great!
I took this PR on thinking that I could address the remaining checks, but ran into hiccups with both:
- Failing unit test check
- There are two storyshots-related snapshots affected by these changes that need updated. I was unable to figure out how to update these locally, after running into issues trying to build
addon-storyshots.
- There are two storyshots-related snapshots affected by these changes that need updated. I was unable to figure out how to update these locally, after running into issues trying to build
- Chromatic UI tests
- There are a few failing stories that use
place-hold.itfor placeholder images, which has apparently become unreliable. - I intended to replace these with a local utility to generate a placeholder image with specific dimensions, but:
- I didn't see a good place for those to live within the
examplesdirectory - There are a lot more calls to that service than just those surfaced by this PR. Felt like it should be a separate effort.
- I didn't see a good place for those to live within the
- There are a few failing stories that use
I'll take a look at this tomorrow!
Fortunately this has been resolved already, I just merged a PR that addressed this issue 🎉 |
|
I'll start adding my findings as comments, hope you don't mind! I wonder why Chromatic didn't pick this up, I guess we don't have a story for such component.. but something went wrong with the spacing of the share button in docs mode:
And this is from an example using |
|
I wonder if the icons becoming smaller is also intended [previous] --------------------------------------------------- [this PR] |
- The snapshot styles changed because the Button component was modified
|
Thanks Yann! Re: icons getting smaller, @MichaelArestad I wonder if we should embiggen the docs icon so that the doc extends to the top and bottom of the area (similar to the profile icon)
re: Docs mode share button
|
I was able to confirm this but only with
This is also a bug in Next so it is unrelated to this PR. You can also see on the official storybook this styling is repeated a few times, which also isn't great. @thafryer can I get some help on this?
|
|
@domyen I tried making the docs icon larger and, after seeing how it looks, recommend sticking with the current version. Alternatively, we could make the component icon smaller (fourth option):
|
|
LGTM @MichaelArestad cc @yannbf
|
What I did
I updated every icon in Storybook. I also reworked the icon component a bit.
How to test
If your answer is yes to any of these, please make sure to include it in your PR.