feat(explore-attr-breakdowns): Using new useChartXRangeSelection hook #103814
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ); | ||
| boxSelectOptions.clearSelection(); | ||
| }, [boxSelectOptions, router]); | ||
| clearSelection(); |
There was a problem hiding this comment.
Bug: Falsy check fails for zero timestamp values
The condition checks if startTimestamp or endTimestamp are falsy, but since these are numeric timestamps, a value of 0 (Unix epoch) is valid yet falsy in JavaScript. This causes the function to incorrectly return early when the selection includes timestamp 0, preventing zoom functionality for that edge case. The check needs to explicitly test for null or undefined rather than relying on falsy evaluation.
| saveOnZoom: true, | ||
| }); | ||
|
|
||
| const {groupName} = useWidgetSyncContext(); |
There was a problem hiding this comment.
useWidgetSyncContext gets called on line 141. Can we just get the groupName from there? Unless there's a reason we need to create another context?
| boxSelectOptions={boxSelectOptions} | ||
| triggerWrapperRef={triggerWrapperRef} | ||
| /> | ||
| {index === 0 ? <AttributeComparisonCTA>{widget}</AttributeComparisonCTA> : widget} |
There was a problem hiding this comment.
Bug: Chart clicks clear selection unexpectedly
The ChartWrapper no longer prevents selection clearing when clicked. The old useChartBoxSelect hook checked if clicks were inside chartWrapperRef before clearing selection, but the new useChartXRangeSelection hook uses data-explore-chart-selection-region attributes instead. The ChartWrapper lacks this attribute, causing clicks on the chart to clear the selection, breaking the previous behavior where users could interact with the chart without losing their selection.
| const {brush, onBrushEnd, onBrushStart, toolBox, ActionMenu} = useChartXRangeSelection({ | ||
| chartRef, | ||
| deps: [props.plottables], | ||
| disabled: true, | ||
| chartsGroupName: groupName, | ||
| ...props.chartXRangeSelection, | ||
| }); |
There was a problem hiding this comment.
I think I missed this in the original PR, but it looks like useChartXRangeSelection accepts a actionMenuRenderer, and then returns the rendered component (ActionMenu) using some states within the hook.
This feels unconventional to me because the hook now handles rendering components outside of echarts as well as brush logic in echarts. I don't think this is a blocker, but would it make more sense for the hook to just return the states necessary for rendering instead? And then the parent can handle the rendering using those states? Unless there is some limitation that forces us to do it this way. Feel free to address in a follow up if necessary!
useChartXRangeSelectionto enable range selection only in explore chartsuseBoxChartSelectionhook