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(brush): add multi axis brushing #625

Merged
merged 12 commits into from
Apr 28, 2020

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Apr 10, 2020

Summary

This commit allows the consumer to configure the direction used by the brush tool from
<Settings brushAxis="x / y / both" /> .
By default, the direction is along the X-axis, but can be configured to go along the Y-axis or along both axes.
For the Y-axis values, we are returning an array of values, one for each Y-axis defined (this is determined by the groupId associated with different axis/series groups)

Along Y axis:

Apr-10-2020 15-15-33

Along both axis, aka rectangular selection:

Apr-10-2020 15-14-56

BREAKING CHANGE: The type used by the BrushEndListener is now changed to reflect this change. The BrushEndListener is changed to

type BrushEndListener = (brushArea: XYBrushArea) => void;

export interface GroupBrushExtent {
  groupId: GroupId;
  extent: [number, number];
}
export interface XYBrushArea {
  x?: [number, number];
  y?: Array<GroupBrushExtent>;
}

fix #587

Summarize your PR.

If it involves visual changes include a screenshot or gif.

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)
  • This was checked for cross-browser compatibility, including a check against IE11
  • 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

This commit allow the consumer to configure the direction used for the brush tool. The direction is
by default along the X axis, but can be changed to be along the Y axis or have a rectangular
selection along both axis. For the Y axis values, we are returning an array of values, one for each
Y axis defined (this is determined by the groupId associated with different axis/series groups)

BREAKING CHANGE: The type used by the `BrushEndListener` is now changed to reflect this change. The
new type is on form of `{ x?: [number, number]; y?: Array<{ groupId: GroupId; values: [number,
number]; }> }` where x contains an array of [min,max] values, and the y property is an optional
array of objects, containing the GroupId and the values of the brush for that specific axis.

fix elastic#587
@markov00 markov00 added :interactions Interactions related issue wip work in progress :xy Bar/Line/Area chart related labels Apr 10, 2020
@codecov-io
Copy link

codecov-io commented Apr 10, 2020

Codecov Report

Merging #625 into master will increase coverage by 0.57%.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
+ Coverage   66.44%   67.01%   +0.57%     
==========================================
  Files         240      252      +12     
  Lines        7012     7228     +216     
  Branches     1340     1328      -12     
==========================================
+ Hits         4659     4844     +185     
- Misses       2337     2367      +30     
- Partials       16       17       +1     
Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/specs.ts 100.00% <ø> (ø)
...t_types/xy_chart/state/selectors/get_brush_area.ts 85.71% <81.25%> (+21.00%) ⬆️
...es/xy_chart/state/selectors/on_brush_end_caller.ts 86.95% <84.48%> (-2.70%) ⬇️
src/scales/scale_band.ts 97.36% <100.00%> (ø)
src/scales/scale_continuous.ts 97.65% <100.00%> (ø)
src/specs/settings.tsx 90.90% <100.00%> (+0.28%) ⬆️
src/utils/commons.ts 100.00% <100.00%> (ø)
src/mocks/scale/index.ts 100.00% <0.00%> (ø)
src/mocks/utils.ts 88.88% <0.00%> (ø)
src/mocks/specs/index.ts 100.00% <0.00%> (ø)
... and 9 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 e9b09ce...7de8739. Read the comment docs.

@markov00 markov00 added enhancement New feature or request and removed wip work in progress labels Apr 15, 2020
@markov00 markov00 self-assigned this Apr 15, 2020
@markov00 markov00 marked this pull request as ready for review April 15, 2020 16:08
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM!! Tested locally in Chrome and Safari . The brush tool story is clear on both browsers and is easy to understand.

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.

Looks good, very nice enhancement. I just had two thoughts while playing around with it.

Thought 1

I noticed that you sort the values for the start and end regardless of which was actually the start. I don't think this should be the case if someone needs to know the order of which the action happened, left to right or right to left.
Screen Recording 2020-04-16 at 09 40 AM

Thought 2

Could you add a minimum height and/or width to the selection overlay? If the user is brushing for any case they can see nothing but the cursor. Just a few pixels maybe.

Screen Recording 2020-04-16 at 09 43 AM

integration/page_objects/common.ts Show resolved Hide resolved
integration/page_objects/common.ts Show resolved Hide resolved
Changing the minBrushDelta value in the Settings component will deny any brush event with less than the specified number of pixel
@markov00
Copy link
Member Author

@nickofthyme I've added the the minBrushDelta prop to <Settings /> to specify a minimum number of pixel to identify a brush area as valid 7de8739
I prefer to keep the brushed values in a sorted array [min, max], then having them as [first, last]
we can then change this behavior later.

@markov00 markov00 requested a review from nickofthyme April 17, 2020 10:25
@nickofthyme
Copy link
Collaborator

nickofthyme commented Apr 20, 2020

@markov00 That's fine for the sort order.

For the min delta, I don't see the min number behavior, It still allows me to make the line invisible. Am I missing something?

Screen Recording 2020-04-20 at 10 58 AM

@monfera
Copy link
Contributor

monfera commented Apr 20, 2020

Disappearing area: users are box selecting during a continuous expression of intent, ie. holding down the mouse, dragging and seeing visual feedback, encountering a special case which shows zero area, which even corresponds to the current intent. The user keeps pressing the mouse button and is likely to move further if wanted. Also, click-hold-drag-release is appropriately done on low-risk interaction ie. there isn't much harm if the user releases the mouse and retries (if there are negative consequences, D&D is not a good activating mechanism to begin with).

Other libraries also have disappearing areas, eg. the lower Highcharts scatter or area, or the Plotly box select which is btw. an auto X/Y/box selector in this example.

It's possible to add eg. corner markers so that they fall outside the actual selected area, but then there are some mundane details to consider, eg. it can protrude outside the Cartesian area if a selection border coincides with an extreme (which is implementable but it may be a time cost/benefit tradeoff).

Besides the effort, code size and complexity, there are some advantages to keeping the visuals simple anyway:

  • clean, simple, yet unambiguous feedback for the user
  • small corner marks not interfering with the mouse cursor (which is why even the Plotly box markers are made to disappear

P.S. lasso and multi-box select are easier without corner markers
P.P.S. I added corner markers to active Kibana Canvas elements, but there it's a resize grab handle, so it was needed.

So it's a fine detail with some tradeoffs that I think wouldn't block this PR, OK done with the bike shedding for a while 🤣

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Looks good! Nice touch: it's possible to select from the very edge of the shown Cartesian area even if 100% pixel accuracy can't be expected of users (Fitts law)

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.

The latest changes are exactly what I expected and look really good.

@markov00 markov00 merged commit 9e49534 into elastic:master Apr 28, 2020
markov00 pushed a commit that referenced this pull request Apr 28, 2020
# [19.0.0](v18.4.2...v19.0.0) (2020-04-28)

### Bug Fixes

* tooltip container scroll issue ([#647](#647)) ([f411771](f411771))
* **annotations:** fix alignment at the edges ([#641](#641)) ([43c5a59](43c5a59)), closes [#586](#586)

### Features

* shift click legend items & partition legend hover ([#648](#648)) ([ed91744](ed91744))
* **brush:** add multi axis brushing ([#625](#625)) ([9e49534](9e49534)), closes [#587](#587) [#620](#620)

### BREAKING CHANGES

* **brush:** The type used by the `BrushEndListener` is now in the following form `{ x?: [number, number]; y?: Array<{ groupId: GroupId; values: [number,
number]; }> }` where `x` contains an array of `[min, max]` values, and the  `y` property is an optional array of objects, containing the `GroupId` and the values of the brush for that specific axis.
* **annotations:** In the rectangular annotation, the y0 parameter of the coordinates now refers to the minimum value and the y1 value refers to the maximum value of the y domain.
@markov00
Copy link
Member Author

🎉 This PR is included in version 19.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 28, 2020
@markov00 markov00 deleted the 2020_04_09-feat_y_brush branch November 25, 2020 11:47
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [19.0.0](elastic/elastic-charts@v18.4.2...v19.0.0) (2020-04-28)

### Bug Fixes

* tooltip container scroll issue ([opensearch-project#647](elastic/elastic-charts#647)) ([f0af34b](elastic/elastic-charts@f0af34b))
* **annotations:** fix alignment at the edges ([opensearch-project#641](elastic/elastic-charts#641)) ([c698d08](elastic/elastic-charts@c698d08)), closes [opensearch-project#586](elastic/elastic-charts#586)

### Features

* shift click legend items & partition legend hover ([opensearch-project#648](elastic/elastic-charts#648)) ([cf15ca1](elastic/elastic-charts@cf15ca1))
* **brush:** add multi axis brushing ([opensearch-project#625](elastic/elastic-charts#625)) ([382cb14](elastic/elastic-charts@382cb14)), closes [opensearch-project#587](elastic/elastic-charts#587) [opensearch-project#620](elastic/elastic-charts#620)

### BREAKING CHANGES

* **brush:** The type used by the `BrushEndListener` is now in the following form `{ x?: [number, number]; y?: Array<{ groupId: GroupId; values: [number,
number]; }> }` where `x` contains an array of `[min, max]` values, and the  `y` property is an optional array of objects, containing the `GroupId` and the values of the brush for that specific axis.
* **annotations:** In the rectangular annotation, the y0 parameter of the coordinates now refers to the minimum value and the y1 value refers to the maximum value of the y domain.
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.

Y-axis brushing
5 participants