Adding new icons to support navigation update #198
Conversation
There was a problem hiding this comment.
Hey Jay! Two things:
-
The SVGs have a
stroke="#3F20BA"on them, which will cause the SVGs to always have this color and makes it impossible for us to change it. Here's the code in the conversion script replaces certain values ("#000" and "#000000" for example) with "currentcolor" so we can control the color externally.space-kit/src/icons/scripts/convert.ts
Lines 188 to 192 in 08fc723
-
This is tribal knowledge that I wish I could have conveyed without having to leave a comment on a PR. Maybe I (or anyone!) should add this to the "add an svg" guide
Please replace the stroke="#3F20BA" with stroke="#000000" and then check out the storybook stories to make sure that the colors are right in all the chromatic changes 😃
|
🚀 PR was released in |
|
@jchesterman in the future, could you please include screenshots of the icons added to your PR description? 🙏 Thanks!! |
|
@jchesterman also, I'm a little confused about something – it appears that you've re-added some icons that we already have in place. Is there any reason we need both, or can we re-use the existing icons to keep from having duplicates in the set? E.g:
As a design principle, my perspective is that we should only have one instance of each icon in SpaceKit. Is there any reason that we might need it more once from your perspective? Completely unrelated, Sorry for all the belated post-ship feedback!! 😊 |
|
@jglovier thanks for all the feedback! I discovered that a lot of the added icons were duplicates today unfortunately :( When looking at the storybook for space kit, I didn't realize there was more icons to the right that I had to horizontally scroll to see, so I made the assumption we were missing a bunch of icons from the dropbox folders, definitely my bad!! I'm going to make another PR tomorrow which will include removing the duplicate icons that were added in the last PR, maybe I can fix the essentials/IconServer stroke weight as well. Upon my discovery today, I also noted there were a couple duplicate icons already in existence, but I can't remove them at this time, because i'm not sure if they're in use anywhere. I will make note of them in the PR though. I'll also make sure to have more descriptive PRs moving forward, thanks again man! |
|
@jchesterman haha, no worries! Yeah that UI can be confusing. Glad to hear it was just a simple misunderstanding of the existing state. 💛
Yeah, we def have a lot of icon cleanup work still to do. I'm working on documenting it in a design debt inventory. Eventually we need to remove the old percolate icons as well, etc. We'll get there! 😅 |
|
Man; I have a todo to update the icons in storybook. I think I'm going to really make each icon it's own story. Then the chromatic change will show exactly what has changed and you can browse the new icons with copy pasting into the PR. |
that might be expensive :P |
You're right; I'd advocate for each icon having one story with each variation (thin/normal weight, special color) and then one story for each icon. Possibly. If we have 200 icons, then 200 shots * 0.006 ($/shot) puts Chromatic at ~$1.20 per run. Even if we have a few hundred runs per month; I don't think that's prohibitive to have some observability as to what's actually changing. |
|
Do we really need the special color variant in Storybook? That might help a bit with reducing cost. In any case, I'm a bit 👍 on each icon being it's own story. |
@jglovier yes we absolutely do. Thinking of the icons as tests, we need to see that setting a color will actually work for each icon. I've caught PRs where the storybook story showed some icons didn't change to teal like we'd expect |
|
Ahhh gotcha. I didn't realize that was the use case. 👌 |
Adding new icons to support navigation update.