feat(addon-docs): named colors with ColorPalette #9453
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/storybook/monorepo/m6trrhpun |
|
👋 @shilman, |
|
Yeah. It's not a bugfix, and 6.0 is the next feature release (end of march, with alphas starting later this week) |
|
Looks good to me and what do you think about the changes? :)
|
| } | ||
| return ( | ||
| <SwatchSpecimen> | ||
| <SwatchColors>{Object.values(colors).map(color => renderSwatch(color))}</SwatchColors> |
There was a problem hiding this comment.
Any concerns regarding order of the design tokens?
There was a problem hiding this comment.
Thank you for your review @RuneKR but I don't get it?
I simply iterate over the object as it is.
I mean if design tokens need to be reordered then, it must be done in the story, I guess.
There was a problem hiding this comment.
Looks like you got the gist of it. I understand that the order is arbitrary in this solution. I was wondering if the solution could/should be extended to allow the developer to specify the order of the colours. This could be achieved by using this data structure:
[{name: 'name of colour', value: 'hex or whatever']
This capability can also be added without making it a breaking change.
There was a problem hiding this comment.
Actually, it's an excellent question!
I still use design tokens starting by $ (because of sass variable conventions) so it does not concern me directly.
It would be great to have the point of view of someone else because I consider that it's not totally necessary. :)
There was a problem hiding this comment.
I feel like that could be enhancement made in a following PR, if desired.
There was a problem hiding this comment.
I wouldn't call the order arbitrary. (Ignoring IE)
https://www.stefanjudis.com/today-i-learned/property-order-is-predictable-in-javascript-objects-since-es2015/
|
Do we need another review, to be able to merge? 🤗 |
|
cc @domyen |
|
I didn't really get a chance to check this out in time (sorry everyone). Some changes requests And this story's layout should not have changed because there are no color names included. |
|
I'm sorry guys, I used to delete the branch once it's merged, I did not realize that demo disappears 😬 |
|
No revert needed, but a follow up PR with @domyen's review comments applied would be great 🙇 |
|
I invited you to the github org @frassinier, you should be able to directly commit on this repo now, no need for a fork. |
|
Thanks, i'm out the office right now, I will do it on next Monday. |
Issue:
Cannot display design token names using ColorPalette docs component
What I did
Be able to pass an object for colors in addition to a simple array.
How to test
NamedColorsstory has been added.