-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Design system/horizontal bar chart #565
Conversation
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.
This looks really great, @bradfordjanice !
My only suggestions were minor.
DATA: "Data", | ||
CUSTOM: "Custom" | ||
}; | ||
const getKeyNames = obj => { |
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.
Since this is reused in BarChart, HorizontalBarChart, and will be used in other stories, extract to shared.js helper file and import rather than duplicating.
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.
I will also extract getKeyNames from the BarChart story and import from shared.js. (In a new branch.)
return ( | ||
<HorizontalBarChart | ||
data={data} | ||
sortOrder={sortOrder} |
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.
Technically, sortOrder isn't required, the component will pick a sortOrder by default. We should move it to the custom story.
A follow up issue for this is that the default sort should be changed to sort from highest value to lowest value, rather than the current sorting method.
Updated the HorizontalBarChart story: organized stories, added notes, created knobs for each story.