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

fix(histogram): fix annotation and tooltip overflow with single value #343

Merged
merged 2 commits into from
Aug 26, 2019

Conversation

markov00
Copy link
Member

Summary

fix #342, fix #341

This commit fix an issue where a rect annotation with x0 defined in a single value histogram, is
drawn outside the chart as shown in #341.
It also fix the tooltip rendering inside the chart: #342

before
Screenshot 2019-08-23 at 01 27 42

after
Screenshot 2019-08-23 at 01 04 38

BREAKING CHANGE
The current coordinate configuration of a rect annotation seems inverted. From the rect annotation PR: #180 I'm proposing to invert the x0 and x1 meaning on the annotation coordinate.
With x0 I always think of the left hand side of a rectangle, and with x1 the right hand side.
So with that in mind I'd like to change that with

x0 x1 y0 y1 render implications
undefined undefined undefined undefined no rendered annotation
defined undefined (coerced to x domain end) undefined (coerced to y domain end) undefined (coerced to y domain start) rect will be drawn from x0 to xDomain end with full chart height
undefined (coerced to x domain start) defined undefined (coerced to y domain end) undefined (coerced to y domain start) rect will be drawn from x domain start to x1 with full chart height
undefined (coerced to x domain start) undefined (coerced to x domain end) defined undefined (coerced to y domain start) rect will be drawn from y domain start to y0 with full chart width
undefined (coerced to x domain start) undefined (coerced to x domain end) undefined (coerced to y domain end) defined rect will be drawn from y1 to y domain end with full chart width

@nickofthyme do you think we should to the same also for the y0 and y1 values?

Checklist

Use strikethroughs to remove checklist items you don't feel are 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
  • Each commit follows the convention

This commit fix an issue where a rect annotation with x0 defined in a single value histogram, is
drawn outisde of the chart.

BREAKING CHANGE: The current coordinate configuration of a rect annotation were inverted. This
commit now reverse them: a rect coordinate with only the x0 value will cover from the x0 value to
the end of the domain, a rect coordinate with only the x1 value will cover the interval from the
beginning of the domain till the x1 value.
@markov00 markov00 added bug Something isn't working :interactions Interactions related issue :annotation Annotation (line, rect, text) related issue labels Aug 22, 2019
@markov00 markov00 requested a review from nickofthyme August 22, 2019 23:56
@markov00 markov00 changed the title fix(histogram): fix overflowing annotation with single value fix(histogram): fix annotation overflow with single value Aug 22, 2019
@codecov-io
Copy link

codecov-io commented Aug 23, 2019

Codecov Report

Merging #343 into master will decrease coverage by 0.03%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
- Coverage   98.34%   98.31%   -0.04%     
==========================================
  Files          38       38              
  Lines        2726     2736      +10     
  Branches      646      647       +1     
==========================================
+ Hits         2681     2690       +9     
- Misses         42       43       +1     
  Partials        3        3
Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/scales.ts 100% <ø> (ø) ⬆️
...art_types/xy_chart/annotations/annotation_utils.ts 100% <100%> (ø) ⬆️
src/utils/scales/scale_continuous.ts 93.75% <91.66%> (-0.32%) ⬇️

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 02a313b...ef308a9. Read the comment docs.

@markov00 markov00 changed the title fix(histogram): fix annotation overflow with single value fix(histogram): fix annotation and tooltip overflow with single value Aug 25, 2019
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.

Tested locally, LGTM.

Ya both seem to be inverted. I think swapping y0 and y1 would be good to stay consistent.

src/utils/scales/scale_continuous.ts Show resolved Hide resolved
@markov00 markov00 merged commit 2268f04 into elastic:master Aug 26, 2019
markov00 pushed a commit that referenced this pull request Aug 26, 2019
# [11.0.0](v10.3.1...v11.0.0) (2019-08-26)

### Bug Fixes

* **histogram:** fix overflowing annotation with single value ([#343](#343)) ([2268f04](2268f04)), closes [#342](#342) [#341](#341)

### BREAKING CHANGES

* **histogram:** The current coordinate configuration of a rect annotation were inverted. This commit now reverse them: a rect coordinate with only the x0 value will cover from the x0 value to the end of the domain, a rect coordinate with only the x1 value will cover the interval from the beginning of the domain till the x1 value.
@markov00
Copy link
Member Author

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Aug 26, 2019
@markov00 markov00 deleted the fix-histogram-annotation-n-tooltip branch November 25, 2020 11:44
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [11.0.0](elastic/elastic-charts@v10.3.1...v11.0.0) (2019-08-26)

### Bug Fixes

* **histogram:** fix overflowing annotation with single value ([opensearch-project#343](elastic/elastic-charts#343)) ([7dc097e](elastic/elastic-charts@7dc097e)), closes [opensearch-project#342](elastic/elastic-charts#342) [opensearch-project#341](elastic/elastic-charts#341)

### BREAKING CHANGES

* **histogram:** The current coordinate configuration of a rect annotation were inverted. This commit now reverse them: a rect coordinate with only the x0 value will cover from the x0 value to the end of the domain, a rect coordinate with only the x1 value will cover the interval from the beginning of the domain till the x1 value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:annotation Annotation (line, rect, text) related issue bug Something isn't working :interactions Interactions related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip outside chart for single value histogram Rect annotation rendered outside chart on histogram
3 participants