Feat: BarChart axis and icons #52
Conversation
adds a label option and icon option to the bar chart
adds axis and graph icon support
|
Woohoo! new surge deployment available for viewing! 🎉 surgereview23604592.surge.sh |
There was a problem hiding this comment.
This is great, thanks!
Oh, one comment - the tooltip for the BarChart has a hardcoded label of "operations"; I think it would be great if that were a prop instead, do you agree? But that could of course be changed outside of this PR.
| /> | ||
| </div> | ||
|
|
||
| Icons are driven from the currentState property of every record. Warning and critical will display icons while 'normal' does not. | ||
| Set the showIcon & showXLabel props to respectively enable those display properties. |
|
|
||
| // const xScale = ChartUtils.xScaleBand(data, innerWidth, barPadding); | ||
| const data = Immutable.fromJS(this.props.data).toJS() | ||
| const xScaleTimeLineData = Immutable.fromJS(this.props.data).toJS() |
There was a problem hiding this comment.
why use immutable here? were essentially just running an identity fn. this is opinion, but I think it's the responsibility of the consumer to manage the mutability of his/her data. if all we're lookin' for is a copy we could consider using Object.assign/clone, and shave a dep!
There was a problem hiding this comment.
This is a bit of carryover from the prototype where we used immutable in the charts. Object.assign isn't a good fit here since this is an array of objects - but I can put in an alternative solution. I'll update the branch with an update that removes the dependency.
|
@jhegg The tooltip should be earmarked for an update when the data that is feeding it is solidified. Right now the props for the tooltip are a best guess from the prototype. I'll add a 'tooltipTitle' prop to the chart that lets you specify a text string for that specific piece you mentioned. |
|
Great, thanks. So I think we're just waiting on the immutable dependency change before merging, right? |
|
That and I'll change the ability to send a prop title to the tooltip component. |
removes dependency on immutable and adds title prop support for tooltip
|
Woohoo! new surge deployment available for viewing! 🎉 surgereview23639783.surge.sh |
|
Awesome, that's looking great! |
Enables axis date display and icons on bar chart. See styleguidist example for usage.