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

new(xychart): add PointerEvent handlers to XYChart and *Series #947

Merged
merged 24 commits into from
Dec 3, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Nov 25, 2020

TODO

🚀 Enhancements

This PR

  • adds the ability to set PointerEvent handlers on XYChart, or on individual *Series (previously the only mouse events which were handled were to show/hide tooltips)
    • onPointerMove/Out/Up are currently supported
      • hoping this will be compatible with focus/blur (a11y), and brush/zoom/pan in the future
  • adds pointerEvents: boolean to all *Series, which enables users to prevent a Series from emitting, or responding to any PointerEvents (including tooltips; see demo below)
  • all mousemove/out + touchmove/end references were simplified to pointermove/out
  • updates the /xychart example to demonstrate onPointerUp => if you click on the chart, the Annotation will move to the nearest Datum in the nearest Series

Usage examples

// set handler on chart
// invoke for nearest Datum in _nearest_ `Series` to event, or nearest Datum in _all_ `Series`
<XYChart onPointerUp={() => ...} pointerEvents={'all' | 'nearest'}>
  <LineSeries {...} />
  <LineSeries {...} />
</XYChart>

// invoke handler for only a specific `Series`
// `XYChart` will capture and emit events when `captureEvents=true`, else Series DOM elements emit events
// note: Tooltips still work for Series regardless of `PointerEvent` handlers, unless pointerEvents={false}
<XYChart captureEvents={true | false}>
  <LineSeries onPointerMove={() => ...}  {...} />
  <LineSeries {...} />
</XYChart>

Implementation notes

  • all events are dispatched through the EventEmitterContext rather than being handled within a given component.
    • In order to scope e.g., BarSeries events to that BarSeries, I added the notion of EventEmitter sources so that a Series can subscribe to only it's own events + events emitted by XYChart
  • Series PointerEvent handlers are set on individual bars, glyphs, or paths (rather than a container to capture events on all of them) to prevent one Series blocking events for another series.

Demo of pointerEvents={false} for all Series except SF (therefore tooltip only contains SF, even though mouse is closer to other AreaSeries)

@kristw @hshoff

svgPoint,
}: PointerEventParams<Datum>) => void;
/** Whether to invoke PointerEvent handlers for all dataKeys, or the nearest dataKey. */
pointerEvents?: 'all' | 'nearest';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe should be called pointerEventDataKey 🤔

Copy link
Collaborator Author

@williaster williaster Nov 25, 2020

Choose a reason for hiding this comment

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

I guess this probably should mirror the implementation and could also support specific dataKey | dataKey[] as well.

EDIT: gonna punt on this until it's a feature request, makes the type-ing more error prone because string | 'all' becomes string.


const handlePointerOut = useCallback(
(params?: HandlerParams) => {
if (params && onPointerOut) onPointerOut(params.event);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because we have to always use the same # of hooks, we can't conditionally create this and have to do the check in the handler. we conditionally return the handler below, tho.

@williaster williaster added this to the 1.2.1 milestone Nov 26, 2020
@coveralls
Copy link

coveralls commented Dec 1, 2020

Pull Request Test Coverage Report for Build 393068437

  • 117 of 124 (94.35%) changed or added relevant lines in 16 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 59.858%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/src/hooks/usePointerEventEmitters.ts 5 8 62.5%
packages/visx-xychart/src/hooks/usePointerEventHandlers.ts 28 32 87.5%
Totals Coverage Status
Change from base Build 383897138: -0.007%
Covered Lines: 1677
Relevant Lines: 2621

💛 - Coveralls

@williaster williaster merged commit dd11a5c into master Dec 3, 2020
@williaster williaster deleted the chris--xychart-handlers branch December 3, 2020 20:05
const entry = dataRegistry.get(key);
const childData = barSeriesChildren.find(child => child.props.dataKey === key)?.props.data;
if (childData && svgPoint && width && height && showTooltip) {
const datum = findNearestStackDatum(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noting as a TODO that this logic was lost in this refactor

@williaster williaster modified the milestones: 1.2.1, 1.3.0 Dec 10, 2020
@coveralls
Copy link

coveralls commented Nov 24, 2024

Pull Request Test Coverage Report for Build 393068437

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 117 of 124 (94.35%) changed or added relevant lines in 16 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.006%) to 59.858%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/src/hooks/usePointerEventEmitters.ts 5 8 62.5%
packages/visx-xychart/src/hooks/usePointerEventHandlers.ts 28 32 87.5%
Totals Coverage Status
Change from base Build 383897138: -0.006%
Covered Lines: 1677
Relevant Lines: 2621

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants