feat(xy): add onPointerUpdate debounce and trigger options #1194

nickofthyme · 2021-06-07T19:04:03Z

Summary

Adds onProjectionUpdate listener to Settings spec to listen for changes to projected cursor values.

BREAKING CHANGE

PointerOverEvent type now extends ProjectedValues and drops value. This effectively replaces value with x, y, smVerticalValue and smHorizontalValue

Issues

Adds feature desired by logs to listen to cursor updated with projected values
closes #906

Checklist

The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
Whenever possible, please check if the closing issue is connected to a running GH project
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
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

src/chart_types/xy_chart/state/selectors/on_pointer_move_caller.ts

nickofthyme · 2021-06-09T16:53:08Z

packages/charts/src/chart_types/xy_chart/state/selectors/on_pointer_move_caller.ts

+          if (onPointerUpdate && hasPointerEventChanged(tempPrev, nextPointerEvent, true))
+            onPointerUpdate(nextPointerEvent);
+
+          if (onProjectionUpdate && values && hasPointerEventChanged(tempPrev, nextPointerEvent, false)) {
+            const oldYValues = yValuesString;
+            yValuesString = values.y.map(({ value }) => value).join(',');
+
+            if (oldYValues !== yValuesString) onProjectionUpdate(values);


@markov00 I'm not sure about this logic. I want to only run this listener when the y values change but the PointerEvent only includes the x values. Should I add the yValues as optional to the PointerEvent or do you think this is ok?

Yep, I think we could just reuse the onPointerUpdate?: PointerUpdateListener;

You can merge the definitions of PointerOverEvent and ProjectedValues to have a global and more intuitive pointer position like:

interface PointerOverEvent extends BasePointerEvent, ProjectedValues{ type: typeof PointerEventType.Over; scale: ScaleContinuousType | ScaleOrdinalType; /** * Unit for event (i.e. `time`, `feet`, `count`, etc.) Not currently used/implemented * @alpha */ unit?: string; // value removed in favour of ProjectedValues x };

Ok thanks, Do you think I should debouce the listener? I'm not really sure how given how the listener is called in the selector.

I haven't used a debounce for the current onPointerUpdate used to sync charts together.
we can try but adding a way to disable/configure the debounce time

let default to 16ms debounce to have 1 event per frame max at 60FPS

See 232f929 for updates the pointer logic. I'll add the debounce in another commit.

Ok debouncing changes are done here 5f085b5

nickofthyme · 2021-06-24T19:05:19Z

packages/charts/src/chart_types/xy_chart/state/selectors/get_projected_pointer_position.ts

+    return { pos: relativePos, value: panelScale.domain[relativePosIndex] ?? null };
  }
  return {
    pos: posWOInitialOuterPadding - panelScale.step * (numOfDomainSteps - 1),
-    value: panelScale.domain[numOfDomainSteps - 1],
+    value: panelScale.domain[numOfDomainSteps - 1] ?? null,


These values could be undefined which does not match the expected PrimativeType

markov00

I don't see much difference between the onPointerUpdate and the onProjectionUpdate
if not the logic of checking the y values.
I think we can easily get rid of the onProjectionUpdate, keeping only the onPointerUpdate.
We can probably improve the code if the user can specify what to listen to:

changes to the x position
changes to the y position
both
In this way, if the users are interested in the x movements, they receive updates only when the x position changed (in particular useful when using large bars where the x only change between bars and large movements). If someone is just interested in the Y space, then they can listen just to y changes.
Finally they can listen for both changes and use the debounce to limit the amount of data received

# [31.0.0](v30.2.0...v31.0.0) (2021-06-29) ### Bug Fixes * **xy:** render gridlines behind axis ([#1204](#1204)) ([38ebe2d](38ebe2d)), closes [#1203](#1203) * memory leak related to re-reselect cache ([#1201](#1201)) ([02025cf](02025cf)) * **partition:** getLegendItemsExtra no longer assumes a singleton ([#1199](#1199)) ([100145b](100145b)) ### Features * **annotations:** option to render rect annotations outside chart ([#1207](#1207)) ([4eda382](4eda382)) * **heatmap:** enable brushing on categorical charts ([#1212](#1212)) ([10c3493](10c3493)), closes [#1170](#1170) [#1171](#1171) * **xy:** add onPointerUpdate debounce and trigger options ([#1194](#1194)) ([a9a9b25](a9a9b25)) ### BREAKING CHANGES * **xy:** the `PointerOverEvent` type now extends `ProjectedValues` and drops value. This effectively replaces value with `x`, `y`, `smVerticalValue` and `smHorizontalValue`.

nickofthyme · 2021-06-29T04:28:25Z

🎉 This PR is included in version 31.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [31.0.0](elastic/elastic-charts@v30.2.0...v31.0.0) (2021-06-29) ### Bug Fixes * **xy:** render gridlines behind axis ([opensearch-project#1204](elastic/elastic-charts#1204)) ([bf9ccbd](elastic/elastic-charts@bf9ccbd)), closes [#1203](elastic/elastic-charts#1203) * memory leak related to re-reselect cache ([opensearch-project#1201](elastic/elastic-charts#1201)) ([8cb6876](elastic/elastic-charts@8cb6876)) * **partition:** getLegendItemsExtra no longer assumes a singleton ([opensearch-project#1199](elastic/elastic-charts#1199)) ([ecbcc1e](elastic/elastic-charts@ecbcc1e)) ### Features * **annotations:** option to render rect annotations outside chart ([opensearch-project#1207](elastic/elastic-charts#1207)) ([ddffc00](elastic/elastic-charts@ddffc00)) * **heatmap:** enable brushing on categorical charts ([opensearch-project#1212](elastic/elastic-charts#1212)) ([5c426b3](elastic/elastic-charts@5c426b3)), closes [opensearch-project#1170](elastic/elastic-charts#1170) [opensearch-project#1171](elastic/elastic-charts#1171) * **xy:** add onPointerUpdate debounce and trigger options ([opensearch-project#1194](elastic/elastic-charts#1194)) ([aa068f6](elastic/elastic-charts@aa068f6)) ### BREAKING CHANGES * **xy:** the `PointerOverEvent` type now extends `ProjectedValues` and drops value. This effectively replaces value with `x`, `y`, `smVerticalValue` and `smHorizontalValue`.

nickofthyme added enhancement New feature or request :interactions Interactions related issue :xy Bar/Line/Area chart related labels Jun 7, 2021

nickofthyme requested a review from markov00 June 7, 2021 19:04

nickofthyme commented Jun 7, 2021

View reviewed changes

src/chart_types/xy_chart/state/selectors/on_pointer_move_caller.ts Outdated Show resolved Hide resolved

feat(xy): add onProjectionUpdate listener

818af54

nickofthyme force-pushed the on-projected-update branch from 34d9a3b to 818af54 Compare June 9, 2021 16:46

nickofthyme commented Jun 9, 2021

View reviewed changes

Merge branch 'master' into on-projected-update

ef95cb4

nickofthyme marked this pull request as ready for review June 23, 2021 15:16

nickofthyme added 3 commits June 24, 2021 10:47

refactor: combine pointer update type and fix all unit tests

232f929

Merge branch 'master' into on-projected-update

f1118dd

refactor: debounce pointer update listeners

5f085b5

nickofthyme commented Jun 24, 2021

View reviewed changes

chore: update api changes

e5df001

markov00 reviewed Jun 25, 2021

View reviewed changes

nickofthyme added 2 commits June 28, 2021 22:26

refactor: merge update listeners into one

cd0b549

test: update snapshots

7c849fd

nickofthyme changed the title ~~feat(xy): add onProjectionUpdate listener~~ feat(xy): add onPointerUpdate debounce and trigger options Jun 29, 2021

nickofthyme merged commit a9a9b25 into elastic:master Jun 29, 2021

nickofthyme deleted the on-projected-update branch June 29, 2021 04:18

nickofthyme added the released Issue released publicly label Jun 29, 2021

markov00 mentioned this pull request Jun 29, 2021

Update dependency @elastic/charts to v31.0.0 (master) elastic/kibana#102078

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xy): add onPointerUpdate debounce and trigger options #1194

feat(xy): add onPointerUpdate debounce and trigger options #1194

nickofthyme commented Jun 7, 2021 •

edited

Loading

nickofthyme Jun 9, 2021

nickofthyme Jun 23, 2021

markov00 Jun 24, 2021

nickofthyme Jun 24, 2021

markov00 Jun 24, 2021

markov00 Jun 24, 2021 •

edited

Loading

nickofthyme Jun 24, 2021

nickofthyme Jun 24, 2021

nickofthyme Jun 24, 2021

markov00 left a comment

nickofthyme commented Jun 29, 2021

feat(xy): add onPointerUpdate debounce and trigger options #1194

feat(xy): add onPointerUpdate debounce and trigger options #1194

Conversation

nickofthyme commented Jun 7, 2021 • edited Loading

Summary

BREAKING CHANGE

Issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

nickofthyme commented Jun 29, 2021

nickofthyme commented Jun 7, 2021 •

edited

Loading

markov00 Jun 24, 2021 •

edited

Loading