-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conglei horizontal bar #127
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 sweet overall! Had a couple of thoughts, and could we add at least one new test that covers the new functionality?
yScale={horizontal ? categoryScale : valueScale} | ||
margin={{ left: 100, top: 64, bottom: 64 }} | ||
> | ||
<PatternLines |
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.
not sure if you want to reference this or not, I like patterns :) I'd update the id if so, or remove component if not.
const color = d.fill || callOrValue(fill, d, i); | ||
const barX = xScale(x(d)) - offset; | ||
const barPosition = categoryScale(categoryField(d)) - offset; | ||
|
||
return ( | ||
isDefined(d.y) && ( |
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.
do you think we should update this to be dependent on horizontal? eg isDefined(horizontal ? d.y : d.x)
|
||
const maxHeight = (yScale.range() || [0])[0]; | ||
const offset = xScale.offset || 0; | ||
const maxBarLength = (valueScale.range() || [0])[0]; |
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.
Math.max(...valueScale.range())
might be a little more robust here
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.
Agreed!
@williaster Tests added. PTAL. Thanks! |
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.
nice, thanks for adding them! LGTM
馃挃 Breaking Changes
馃弳 Enhancements
Added horizontal control for BarSeries
馃摐 Documentation
馃悰 Bug Fix
馃彔 Internal