Make brush send events #5663
Make brush send events #5663
Conversation
| @@ -179,7 +179,7 @@ export const visTypes = { | |||
| expanded: true, | |||
| controlSetRows: [ | |||
| ['color_scheme'], | |||
| ['show_brush', 'show_legend'], | |||
| ['show_brush', 'send_time_range', 'show_legend'], | |||
There was a problem hiding this comment.
I know we rarely use 3 elements in the same row, but I think it works in this case:
There was a problem hiding this comment.
Checkboxes tend to look bad when combined to other controls, I'm not sure how to fix that.
There was a problem hiding this comment.
Can you convert Show Range Filter into another checkbox?
There was a problem hiding this comment.
@kristw Show Range Filter has yes|no|auto options ...
There was a problem hiding this comment.
Ah. I see. Thanks @mistercrunch
I would recommend having both checkboxes on another line then.
| @@ -363,6 +363,13 @@ export default function nvd3Vis(slice, payload) { | |||
| throw new Error('Unrecognized visualization for nvd3' + vizType); | |||
| } | |||
|
|
|||
| if (isTruthy(fd.show_brush) && isTruthy(fd.send_time_range)) { | |||
There was a problem hiding this comment.
NIT: I've seen !!foo used in places as an isTruthy not sure if there are pros/cons but we should aim at consistency throughout the codebase. Not sure if eslint rules may exist for this. @williaster what you think?
| @@ -363,6 +363,13 @@ export default function nvd3Vis(slice, payload) { | |||
| throw new Error('Unrecognized visualization for nvd3' + vizType); | |||
| } | |||
|
|
|||
| if (isTruthy(fd.show_brush) && isTruthy(fd.send_time_range)) { | |||
| chart.focus.dispatch.on('brush', (event) => { | |||
| const timeRange = event.extent.map(d => d.toISOString().slice(0, -1)).join(' : '); | |||
There was a problem hiding this comment.
This would raise if we ever supported focus on non-time xAxis. Can we make sure that d is a date as a condition to trigger the event?
Codecov Report
@@ Coverage Diff @@
## master #5663 +/- ##
==========================================
- Coverage 63.49% 63.48% -0.02%
==========================================
Files 360 360
Lines 22889 22895 +6
Branches 2549 2551 +2
==========================================
Hits 14534 14534
- Misses 8340 8346 +6
Partials 15 15
Continue to review full report at Codecov.
|
| if (extent.some(d => d.toISOString === undefined)) { | ||
| return; | ||
| } | ||
| const timeRange = extent.map(d => d.toISOString().slice(0, -1)).join(' : '); |
There was a problem hiding this comment.
Can we make sure d is a date here as we may support non-date xAxis in the future.
There was a problem hiding this comment.
We check that in line 369, by looking for the existence of the toISOString method.
* Make brush send events * Make sure object is date * Send event only on brushend (cherry picked from commit 2a8cd43)
* Make brush send events * Make sure object is date * Send event only on brushend (cherry picked from commit 2a8cd43)
* Make brush send events * Make sure object is date * Send event only on brushend
mistercrunch
added
🏷️ bot
I added a new option to time series, allowing the time brush (range filter) to send the selection to other charts:
Demo: