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: add projection click listener #913

Merged

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Nov 24, 2020

Summary

This PR adds a projection click listener called onProjectionListener and available in the <Settings /> component with the following characteristics:

  • it will fire every time the user clicks on the projection area (everything within the axes)
  • the listener is called with an object that contains the nearest X and Ys values. These values are not screen coordinates but the real data domain values inverted computed inverting the x and y scales.
  • we don't actually prevent firing the onProjectionListener if an onElementClick listener is configured and the user clicks directly on a bar/point. Both events could fire one after the other. We prevent the onProjectionListener from firing an event if a onElementClick listener is available and it has fired a click event

The type signature of this new listener is the following:

/**
 * @public
 * The listener type for click on the projection area.
 */
export type ProjectionClickListener = (values: ProjectedValues) => void;


/**
 * @public
 * An object that contains the scaled mouse position based on
 * the current chart configuration.
 */
export type ProjectedValues = {
  /**
   * The independent variable of the chart
   */
  x: PrimitiveValue;
  /**
   * The set of dependent variable, each one with its own groupId
   */
  y: Array<{ value: PrimitiveValue; groupId: string }>;
  /**
   * The categorical value used for the vertical placement of the chart
   * in a small multiple layout
   */
  smVerticalValue: PrimitiveValue;
  /**
   * The categorical value used for the horizontal placement of the chart
   * in a small multiple layout
   */
  smHorizontalValue: PrimitiveValue;
};

You can enable it through the <Settings onProjectionListener={yourListenerImplementationHere}/>

The Interactions -> Bar clicks story in storybook is updated to fire with this listener

close #846

Checklist

Delete any items that are not 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)
  • 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

@markov00 markov00 added enhancement New feature or request :interactions Interactions related issue :xy Bar/Line/Area chart related labels Nov 24, 2020
@markov00 markov00 requested a review from nickofthyme November 24, 2020 14:24
@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #913 (5d203f2) into master (860224e) will increase coverage by 0.08%.
The diff coverage is 63.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #913      +/-   ##
==========================================
+ Coverage   70.11%   70.20%   +0.08%     
==========================================
  Files         340      357      +17     
  Lines       10979    10645     -334     
  Branches     2382     2163     -219     
==========================================
- Hits         7698     7473     -225     
+ Misses       3267     3089     -178     
- Partials       14       83      +69     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/specs/settings.tsx 86.66% <ø> (-0.44%) ⬇️
..._types/xy_chart/state/selectors/on_click_caller.ts 57.57% <39.13%> (ø)
...pes/xy_chart/state/selectors/get_cursor_pointer.ts 68.18% <50.00%> (-5.51%) ⬇️
...art/state/selectors/get_projected_scaled_values.ts 93.75% <93.75%> (ø)
src/chart_types/xy_chart/state/chart_state.tsx 100.00% <100.00%> (ø)
src/utils/dimensions.ts 71.42% <0.00%> (-28.58%) ⬇️
src/chart_types/xy_chart/utils/panel_utils.ts 72.72% <0.00%> (-19.59%) ⬇️
src/utils/data_generators/data_generator.ts 45.45% <0.00%> (-9.10%) ⬇️
...t/state/selectors/compute_small_multiple_scales.ts 86.95% <0.00%> (-8.70%) ⬇️
src/components/portal/utils.ts 21.42% <0.00%> (-7.15%) ⬇️
... and 196 more

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 860224e...5d203f2. Read the comment docs.

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM just two comments.

Comment on lines 44 to 47
(lastClick, { onProjectionClick }, values): void => {
if (!onProjectionClick) {
return;
}
Copy link
Collaborator

@nickofthyme nickofthyme Nov 24, 2020

Choose a reason for hiding this comment

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

is there a way to check if the onElementClick was called with the same x value, and if so block this from firing?

I'm just thinking about the use case of having the same action fired for both onElementClick and onProjectionClick especially with multiple bars. Which could fire the action twice. The user could just debounce the function for 100 ms but I wonder if not firing a duplicate event, namely onProjectionClick, would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a way, we should merge the element and projection click handlers and use a single method to handle both cases. I can check if we can quickly implement that in this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, if it's more hassle than it's worth, I think this is fine as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

done here 5d203f2

@markov00 markov00 merged commit 0fa9072 into elastic:master Nov 25, 2020
@markov00 markov00 deleted the 2020_11_24-feat_add_projection_click branch November 25, 2020 16:59
markov00 pushed a commit that referenced this pull request Nov 25, 2020
# [24.2.0](v24.1.0...v24.2.0) (2020-11-25)

### Bug Fixes

* near and far alignments for orthogonal rotations ([#911](#911)) ([cb279f3](cb279f3))

### Features

* add projection click listener ([#913](#913)) ([0fa9072](0fa9072)), closes [#846](#846)
@markov00
Copy link
Member Author

🎉 This PR is included in version 24.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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
Development

Successfully merging this pull request may close these issues.

Enable click on the whole bucket
3 participants