Skip to content
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

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

Merged
merged 8 commits into from
Jun 29, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Jun 7, 2021

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

@nickofthyme nickofthyme added enhancement New feature or request :interactions Interactions related issue :xy Bar/Line/Area chart related labels Jun 7, 2021
@nickofthyme nickofthyme requested a review from markov00 June 7, 2021 19:04
@nickofthyme nickofthyme force-pushed the on-projected-update branch from 34d9a3b to 818af54 Compare June 9, 2021 16:46
Comment on lines 140 to 147
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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@markov00 markov00 Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok debouncing changes are done here 5f085b5

@nickofthyme nickofthyme marked this pull request as ready for review June 23, 2021 15:16
Comment on lines +95 to +99
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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nickofthyme nickofthyme changed the title feat(xy): add onProjectionUpdate listener feat(xy): add onPointerUpdate debounce and trigger options Jun 29, 2021
@nickofthyme nickofthyme merged commit a9a9b25 into elastic:master Jun 29, 2021
@nickofthyme nickofthyme deleted the on-projected-update branch June 29, 2021 04:18
nickofthyme pushed a commit that referenced this pull request Jun 29, 2021
# [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
Copy link
Collaborator Author

🎉 This PR is included in version 31.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Jun 29, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :interactions Interactions related issue released Issue released publicly :xy Bar/Line/Area chart related
Projects
None yet
2 participants