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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jun 29, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
818af54
feat(xy): add onProjectionUpdate listener
nickofthyme Jun 7, 2021
ef95cb4
Merge branch 'master' into on-projected-update
nickofthyme Jun 23, 2021
232f929
refactor: combine pointer update type and fix all unit tests
nickofthyme Jun 24, 2021
f1118dd
Merge branch 'master' into on-projected-update
nickofthyme Jun 24, 2021
5f085b5
refactor: debounce pointer update listeners
nickofthyme Jun 24, 2021
e5df001
chore: update api changes
nickofthyme Jun 24, 2021
cd0b549
refactor: merge update listeners into one
nickofthyme Jun 29, 2021
7c849fd
test: update snapshots
nickofthyme Jun 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/charts/api/charts.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,6 +1545,9 @@ export type ProjectedValues = {
// @public
export type ProjectionClickListener = (values: ProjectedValues) => void;

// @public
export type ProjectionUpdateListener = (values: ProjectedValues) => void;

// @public
export type Ratio = number;

Expand Down Expand Up @@ -1784,6 +1787,7 @@ export interface SettingsSpec extends Spec, LegendSpec {
// (undocumented)
onPointerUpdate?: PointerUpdateListener;
onProjectionClick?: ProjectionClickListener;
onProjectionUpdate?: ProjectionUpdateListener;
// (undocumented)
onRenderChange?: RenderChangeListener;
orderOrdinalBinsBy?: OrderBy;
Expand Down
32 changes: 25 additions & 7 deletions packages/charts/src/chart_types/xy_chart/state/selectors/on_pointer_move_caller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { computeSeriesGeometriesSelector } from './compute_series_geometries';
import { getGeometriesIndexKeysSelector } from './get_geometries_index_keys';
import { getOrientedProjectedPointerPositionSelector } from './get_oriented_projected_pointer_position';
import { PointerPosition } from './get_projected_pointer_position';
import { getProjectedScaledValues } from './get_projected_scaled_values';

const getPointerEventSelector = createCachedSelector(
[
Expand All @@ -49,7 +50,7 @@ function getPointerEvent(
xScale: Scale | undefined,
geometriesIndexKeys: any[],
): PointerEvent {
// update che cursorBandPosition based on chart configuration
// update the cursorBandPosition based on chart configuration
if (!xScale) {
return {
chartId,
Expand Down Expand Up @@ -79,7 +80,11 @@ function getPointerEvent(
};
}

function hasPointerEventChanged(prevPointerEvent: PointerEvent, nextPointerEvent: PointerEvent | null) {
function hasPointerEventChanged(
prevPointerEvent: PointerEvent,
nextPointerEvent: PointerEvent | null,
compareValue: boolean,
) {
if (nextPointerEvent && prevPointerEvent.type !== nextPointerEvent.type) {
return true;
}
Expand All @@ -95,7 +100,8 @@ function hasPointerEventChanged(prevPointerEvent: PointerEvent, nextPointerEvent
nextPointerEvent &&
prevPointerEvent.type === PointerEventType.Over &&
nextPointerEvent.type === PointerEventType.Over &&
(prevPointerEvent.value !== nextPointerEvent.value ||
(!compareValue ||
prevPointerEvent.value !== nextPointerEvent.value ||
prevPointerEvent.scale !== nextPointerEvent.scale ||
prevPointerEvent.unit !== nextPointerEvent.unit)
) {
Expand All @@ -106,13 +112,19 @@ function hasPointerEventChanged(prevPointerEvent: PointerEvent, nextPointerEvent

/** @internal */
export function createOnPointerMoveCaller(): (state: GlobalChartState) => void {
let yValuesString: string = '';
let prevPointerEvent: PointerEvent | null = null;
let selector: Selector<GlobalChartState, void> | null = null;
return (state: GlobalChartState) => {
if (selector === null && state.chartType === ChartType.XYAxis) {
selector = createCachedSelector(
[getSettingsSpecSelector, getPointerEventSelector, getChartIdSelector],
(settings: SettingsSpec, nextPointerEvent: PointerEvent, chartId: string): void => {
[getSettingsSpecSelector, getPointerEventSelector, getChartIdSelector, getProjectedScaledValues],
(
{ onPointerUpdate, onProjectionUpdate }: SettingsSpec,
nextPointerEvent: PointerEvent,
chartId: string,
values,
): void => {
if (prevPointerEvent === null) {
prevPointerEvent = {
chartId,
Expand All @@ -125,8 +137,14 @@ export function createOnPointerMoveCaller(): (state: GlobalChartState) => void {
// we have to update the prevPointerEvents before possibly calling the onPointerUpdate
// to avoid a recursive loop of calls caused by the impossibility to update the prevPointerEvent
prevPointerEvent = nextPointerEvent;
if (settings && settings.onPointerUpdate && hasPointerEventChanged(tempPrev, nextPointerEvent)) {
settings.onPointerUpdate(nextPointerEvent);
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.

@markov00?

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 •
edited
Loading

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

}
},
)({
Expand Down
11 changes: 11 additions & 0 deletions packages/charts/src/specs/settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ export type ProjectedValues = {
* The listener type for click on the projection area.
*/
export type ProjectionClickListener = (values: ProjectedValues) => void;
/**
* @public
* The listener type for mouse move on the projection area.
*/
export type ProjectionUpdateListener = (values: ProjectedValues) => void;

/** @public */
export type ElementClickListener = (
Expand Down Expand Up @@ -496,6 +501,12 @@ export interface SettingsSpec extends Spec, LegendSpec {
* X axis point, and an array of Y values for every groupId used in the chart.
*/
onProjectionClick?: ProjectionClickListener;
/**
* Attach a listener for mouse move on the projection area.
* The listener will be called with the current x value snapped to the closest
* X axis point, and an array of Y values for every groupId used in the chart.
*/
onProjectionUpdate?: ProjectionUpdateListener;
onElementClick?: ElementClickListener;
onElementOver?: ElementOverListener;
onElementOut?: BasicListener;
Expand Down
1 change: 1 addition & 0 deletions stories/interactions/16_cursor_update_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export const Example = () => {
showLegend
showLegendExtra
onPointerUpdate={pointerUpdate}
onProjectionUpdate={action('onProjectionUpdate')}
externalPointerEvents={{
tooltip: { visible: topVisible, placement: topPlacement },
}}
Expand Down