[UI]added reset support and click support for Brush #131
Conversation
conglei
force-pushed
the
conglei_add_brush_reset
branch
from
e878484
to
4cc4a7d
Compare
conglei
changed the title
|
Thanks for those work!!! |
| handleBrushStart(point) { | ||
| const { x, y } = point; | ||
| const { onBrushStart, xScale, yScale } = this.props; | ||
| const invertedX = xScale.invert(x); |
There was a problem hiding this comment.
does this work with band scales?
possible solution here if not (or we could punt to future PR / create issue).
There was a problem hiding this comment.
good catch! Indeed it doesnt work for band scales.. will revise it in this PR.
|
This looks good to me overall, and I pulled it / had no issues locally 馃憤 I think it'd be great to get a test or two in for it if possible tho. |
|
@williaster Added ordinal scale support.
|
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
==========================================
+ Coverage 79.64% 79.76% +0.11%
==========================================
Files 109 109
Lines 2334 2382 +48
Branches 544 547 +3
==========================================
+ Hits 1859 1900 +41
- Misses 301 310 +9
+ Partials 174 172 -2
Continue to review full report at Codecov.
|
| @@ -77,6 +77,27 @@ export function propOrFallback(props, propName, fallback) { | |||
| return props && isDefined(props[propName]) ? props[propName] : fallback; | |||
| } | |||
|
|
|||
| export function scaleInvert(scale, value) { | |||
| if (!scale.invert) { | |||
There was a problem hiding this comment.
could we just add a comment here that band scales don't have the invert method?
| if (!scale.invert) { | ||
| const leftEdges = scale.range(); | ||
| let i = 0; | ||
| const width = scale(scale.domain()[1]) - scale(scale.domain()[0]); |
There was a problem hiding this comment.
could this be replaced with scale.step since this should only be hit for band scales?
| const invertedY0 = yScale.invert(y0 + (y0 < y1 ? -SAFE_PIXEL : SAFE_PIXEL)); | ||
| const invertedY1 = yScale.invert(y1 + (y1 < y0 ? -SAFE_PIXEL : SAFE_PIXEL)); | ||
|
|
||
| let xDomain; |
There was a problem hiding this comment.
I see what you mean about duplicate code, could this be pulled out into a helper function that does the logic that's duplicated for x + y now?
I think you're right that this will have to return an array of values for a band scale.
| } else { | ||
| const xValues = []; | ||
| for (let i = startX; i <= endX; i += 1) { | ||
| xValues.push(xScale.domain()[i]); |
There was a problem hiding this comment.
could call .domain once and assign to a variable here instead of calling multiple times.
conglei
force-pushed
the
conglei_add_brush_reset
branch
from
4e75725
to
d209849
Compare
|
@williaster Added more test and fixed the comment you mentioned :) Thanks for the suggestions :) |
|
this isn't reflecting the successful build, gonna merge. thanks for addressing comments + adding tests! we gotta get that number back up above 80% 馃搱 |
馃挃 Breaking Changes
馃弳 Enhancements
Exposed the reset function to Brush and added an example to show how to use it.
Enabled the click event for
Brush.Exposed starting point of brush to
onBrushStart.馃摐 Documentation
馃悰 Bug Fix
馃彔 Internal