Legend #189
Legend #189
Conversation
angelanicholas
commented
Jan 18, 2017
angelanicholas
commented
- adds new victory legend component & tests
- demos:
Add initial victory-legend
* master: (164 commits) short cut equality checks simplify area add sCU to VictoryLabel adjust domain for padding when calculating padding add sCU to all primitive components Version 11.0.1 - VictorySharedEvents bug update changelog fallback container removed .DS_Store from project Version 11.0.0 - support VictorySelectionContainer update changelog set active on data when triggering toolitps add selection spec support active prop on primitive children add active dont include parent with all eventKey helper allow childName 0 correct child-parent events lint extract selection logic ...
There was a problem hiding this comment.
@angelanicholas this is looking good. I think the changes I would like to see before merging are 1) getting rid of state, and 2) making it work with themes (and then adding appropriate legend themes). If you get around to adding a colorScale that's great too, but a nice to have. Events can wait until v2 if you like.
| import VictoryContainer from "../victory-container/victory-container"; | ||
| import Point from "../victory-primitives/point"; | ||
|
|
||
| const defaultStyles = { |
There was a problem hiding this comment.
@angelanicholas Can this stuff go into a theme instead? Can you add a theme prop so that the legends work with themes?
|
|
||
| constructor(props) { | ||
| super(props); | ||
| this.state = this.getLegendState(props); |
There was a problem hiding this comment.
Other components calculate values in render and then pass some kind of calculatedProps object down to the methods that need them.
| symbol: PropTypes.object | ||
| }) | ||
| ), | ||
| containerComponent: PropTypes.element, |
There was a problem hiding this comment.
It would be nice to have legend take a colorScale prop so that it can auto-color a legend to match some component that uses one of our color scales i.e. pies and stacked bars
| }) | ||
| ), | ||
| containerComponent: PropTypes.element, | ||
| dataComponent: PropTypes.element, |
There was a problem hiding this comment.
VictoryLegend should work with events, but we can save that for v2 if you want.
|
@boygirl Sounds good! I haven't really figured out what kind of events we'd want in legend yet, will file a ticket for that after merging. Removed state and added colorScale and theme props though! Ready for re-review. |
| getCalculatedProps() { // eslint-disable-line max-statements | ||
| const { role } = this.constructor; | ||
| const { data, orientation, theme } = this.props; | ||
| let { height, width } = this.props; |
There was a problem hiding this comment.
You should be able to define width / height in the theme too.
| const symbolStyles = []; | ||
| const labelStyles = []; | ||
| let leftOffset = 0; | ||
|
|
||
| padding = Helpers.getPadding({ padding: padding || theme.padding }); |
There was a problem hiding this comment.
You'll need to remove padding from default props, and make this check respect padding = 0. Also, if you remove padding from default props, this should have a fallback right in the code like props.padding || theme.padding || 0; except with existence checking that respects zero.
There was a problem hiding this comment.
Helpers.getPadding has a 0 fallback already if padding || theme.padding is undefined, and padding was removed from defaultProps in the previous commit
|
🚀 Thanks @angelanicholas |
