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

fix(xy): occlude points outside of y domain #1475

Merged
merged 13 commits into from
Nov 18, 2021
Merged

fix(xy): occlude points outside of y domain #1475

merged 13 commits into from
Nov 18, 2021

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Nov 10, 2021 •
edited
Loading

Summary

Points are removed from rendering that are beyond the yDomain. Tooltips will still show the points outside of the domain.

Details

Issues

Fixes #1474

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • The proper documentation and/or storybook story has been added or updated
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@rshen91 rshen91 marked this pull request as draft November 10, 2021 16:49
@rshen91 rshen91 changed the title fix(xy): occlude points outside of domain fix(xy): occlude points outside of y domain Nov 10, 2021
nickofthyme
nickofthyme reviewed Nov 10, 2021
View reviewed changes
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.

Could you also add a vrt that hovers at x = 2 so we show the tooltip is not available at that point? I think adding it to the existing ones is fine since it can test both the point is no visible AND the tooltip is not shown.

storybook/stories/mixed/8_test_points_outside_domain.story.tsx Outdated Show resolved Hide resolved
integration/tests/mixed_stories.test.ts Outdated Show resolved Hide resolved
integration/tests/mixed_stories.test.ts Outdated Show resolved Hide resolved
integration/tests/mixed_stories.test.ts Outdated Show resolved Hide resolved
@rshen91 rshen91 marked this pull request as ready for review November 10, 2021 17:49
@rshen91
Copy link
Contributor Author

rshen91 commented Nov 10, 2021

Could you also add a vrt that hovers at x = 2 so we show the tooltip is not available at that point? I think adding it to the existing ones is fine since it can test both the point is no visible AND the tooltip is not shown.

Sorry missed this and added in commit 03b6400

@monfera monfera self-requested a review November 10, 2021 21:59
monfera
monfera approved these changes Nov 10, 2021
View reviewed changes
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.

Rachel, this is nicely done, compact change and a good set of image tests to lock in the fix 👍 Looks good to me once CI is in green

Wondering if any of the brush or hover events emit granular, point by point details, and if so, whether the hidden points need to be part of the point set passed on to these callbacks or not

@markov00
Copy link
Member

@monfera

Wondering if any of the brush or hover events emit granular, point by point details, and if so, whether the hidden points need to be part of the point set passed on to these callbacks or not

The brush event doesn't pass the data points details but only the brush extent in domain coordinates.
The hover event only happens when your mouse is actually over the geometry (point in this case) and this is impossible in our situation.

markov00
markov00 reviewed Nov 16, 2021
View reviewed changes
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.

It looks good to me but I'm wondering if we want to hide the tooltip for those values or not.
In particular I see the benefit of showing, in the "cursor" tooltip, also values that are outside the current specified domain, but I think we should investigate with the Lens team if we want or not show those values.
Instead, is definitely a requirement to not render the points that are outside the domain, to avoid having points floating around outside the chart area

@markov00
Copy link
Member

@rshen91 @nickofthyme I've double-checked with @dej611 and for now it is better to keep the point on the tooltip but remove it from the rendering

nickofthyme
nickofthyme approved these changes Nov 18, 2021
View reviewed changes
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.

Code changes since LGTM

storybook/stories/test_cases/8_test_points_outside_of_domain.story.tsx
) : (
<AreaSeries
id="areas"
xScaleType={ScaleType.Linear}
yScaleType={ScaleType.Linear}
xAccessor={0}
yAccessors={[1]}
y0Accessors={[([, y]) => y - 1]}
y0Accessors={showY0Accessor ? [([, y]) => y - 1] : undefined}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏼

packages/charts/src/chart_types/xy_chart/rendering/points.ts
if (isInYDomain) {
indexedGeometryMap.set(pointGeometry, geometryType);
}
indexedGeometryMap.set(pointGeometry, geometryType);
Copy link
Collaborator

@nickofthyme nickofthyme Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍🏼 This is causing vrt changes that need to be updated.

markov00
markov00 approved these changes Nov 18, 2021
View reviewed changes
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.

Looks good to me!

@rshen91 rshen91 merged commit 3176f02 into elastic:master Nov 18, 2021
@rshen91 rshen91 deleted the points-outside-domain branch November 18, 2021 17:19
nickofthyme pushed a commit that referenced this pull request Nov 18, 2021
@semantic-release-bot
chore(release): 40.0.0 [skip ci]
ff4e910 
# [40.0.0](v39.0.2...v40.0.0) (2021-11-18)

### Bug Fixes

* **interactions:** remove the option for pixelRatio with png snapshot ([#1431](#1431)) ([eebb069](eebb069))
* **xy:** occlude points outside of y domain ([#1475](#1475)) ([3176f02](3176f02))

### Features

* **annotations:** add annotations to DebugState ([#1434](#1434)) ([c5ea600](c5ea600))
* **heatmap:** add valueShown in heatmap debug state ([#1460](#1460)) ([962e089](962e089))

### BREAKING CHANGES

* **interactions:** The getPNGSnapshot function no longer has an option for pixelRatio
@u971M u971M mentioned this pull request Oct 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickofthyme nickofthyme nickofthyme approved these changes

@monfera monfera monfera approved these changes

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[XY] Points outside Y domain can be rendered in view
4 participants
@rshen91 @markov00 @monfera @nickofthyme