-
Notifications
You must be signed in to change notification settings - Fork 113
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pan capability to charts #4186
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.
Seems to function well. I do have a question about the implementation, though.
Could you not replace the isRangeValid
and canPanLeft
/canPanRight
with something like this:
$: startRange = $timeControlsStore?.allTimeRange?.start;
$: endRange = $timeControlsStore?.allTimeRange?.end;
$: selectedStart = $timeControlsStore.selectedTimeRange?.start;
$: selectedEnd = $timeControlsStore.selectedTimeRange?.end;
$: canPan = {
left:
(selectedStart?.getTime() || Infinity) >=
(startRange?.getTime() || -Infinity),
right:
(selectedEnd?.getTime() || -Infinity) <=
(endRange?.getTime() || Infinity),
};
That seems to be giving me the same results.
It would be great to hoist this out of the component, too. It only needs to be calculated once, but it's actually being recalculated every time you hover over the chart (though this is mostly because of the hovering
check on line 60/61).
Also, we should put the select-none
class on the MeasureChart div
wrapper.
@djbarnwal Looks good! Approved, but I'll let you merge in case you're still finishing up the testing tasks. |
There's some more changes coming in for Chart interactions. I would merge this now and make the changes in the a later PR. |
Checklist
Closes #4131
Details
Adds Pan interaction to chart using icon button and left-right key press
Steps to Verify