fix: handle chart click as mouseUp to prevent click while brushing

[emmacunningham]

Summary

This PR implements a fix for the issue of the onElementClick handler being called onBrushEnd.

(In the Discover histogram, onElementClick and onBrushEnd are defined for the chart because a bar can be clickable to set the timefilter and the brush tool can also be used to set the timefilter. This bug causes the onElementClick handler to take precendence as it will be called after the onBrushEnd handler is called, resulting in a brush event being overridden by a click on a bar.)

Before: (note how onElementClick is registered after onBrushEnd)

After: (no onElementClick after onBrushEnd)

The source of this issue was that in the previous implementation, the onElementClick callback was called onClick; when both onBrushEnd and onElementClick are defined for a chart, then, the onClick event would be captured on mouse up, including on mouse up after the user has been brushing.

Preventing this required changing the onClick to onMouseUp and within the onMouseUp handler, checking if the chartStore's isBrushing property is true; when true, we return so that the event can be handled as the end of a brush event, and when false, we call the onElementClick callback. The reason for changing from onClick to onMouseUp is because we need to be able to check the isBrushing property; with onClick, the brush end handler has already set isBrushing to false (as this happens onMouseUp) and with onMouseDown, the brush start handler has not yet set isBrushing to true.

The other change is that we set isBrushing to true not right after onBrushStart but rather onBrushing. This was necessary because if we set isBrushing to true immediately onBrushStart (which is handled onMouseDown of the brush layer), then isBrushing would always be true after clicking–even when not brushing and just clicking. So we set isBrushing to true only onBrushing, that is, only onMouseMove followed by onMouseDown.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [] Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)

• This was checked for cross-browser compatibility, including a check against IE11
• Proper documentation or storybook story was added for features that require explanation or tutorials
• Unit tests were updated or added to match the most common scenarios
• Each commit follows the convention

[codecov-io]

Codecov Report

Merging #269 into master will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
+ Coverage   98.12%   98.26%   +0.13%     
==========================================
  Files          37       37              
  Lines        2670     2939     +269     
  Branches      612      718     +106     
==========================================
+ Hits         2620     2888     +268     
- Misses         45       46       +1     
  Partials        5        5

Impacted Files	Coverage Δ
src/lib/series/legend.ts	100% <0%> (ø)	⬆️
src/state/chart_state.ts	98% <0%> (+0.53%)	⬆️
src/state/utils.ts	97.65% <0%> (+0.97%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f196560...abaecf2. Read the comment docs.

[nickofthyme]

Tested locally, LGTM 👍

[emmacunningham]

jenkins test this please

[markov00]

🎉 This PR is included in version 8.1.1 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀