fix(annotations): fix alignment at the edges by markov00 · Pull Request #641 · elastic/elastic-charts

markov00 · 2020-04-21T13:13:44Z

Summary

This commit fixed the alignment of rect and line annotations near the edge of a data domain. It
aligns the x/y position specified by the dataValues of the annotation to the domain of the chart.

BREAKING CHANGE: 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.

fix #586

Checklist

Delete any items that are not applicable to this PR.

Unit tests were updated or added to match the most common scenarios

codecov-io · 2020-04-24T13:41:20Z

Codecov Report

Merging #641 into master will increase coverage by 0.31%.
The diff coverage is 87.11%.

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   72.43%   72.74%   +0.31%     
==========================================
  Files         247      264      +17     
  Lines        8184     8487     +303     
  Branches     1605     1655      +50     
==========================================
+ Hits         5928     6174     +246     
- Misses       2223     2275      +52     
- Partials       33       38       +5

Impacted Files	Coverage Δ
src/chart_types/xy_chart/legend/legend.ts	`84.61% <ø> (-0.39%)`	⬇️
...ypes/xy_chart/renderer/canvas/annotations/index.ts	`30.00% <ø> (ø)`
...ypes/xy_chart/renderer/canvas/annotations/lines.ts	`25.00% <ø> (ø)`
...types/xy_chart/renderer/canvas/annotations/rect.ts	`23.52% <ø> (ø)`
..._types/xy_chart/renderer/canvas/primitives/rect.ts	`5.35% <ø> (ø)`
.../chart_types/xy_chart/renderer/canvas/xy_chart.tsx	`86.30% <ø> (ø)`
src/chart_types/xy_chart/utils/specs.ts	`100.00% <ø> (ø)`
src/scales/index.ts	`100.00% <ø> (ø)`
src/chart_types/xy_chart/annotations/tooltip.ts	`64.70% <64.70%> (ø)`
src/mocks/specs/specs.ts	`74.71% <79.16%> (ø)`
... and 43 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 f411771...4f3e295. Read the comment docs.

This commit fixed the alignment of rect and line annotations near the edge of a data domain. It aligns the x/y position specified by the dataValues of the annotation to the domain of the chart. BREAKING CHANGE: 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. Previously, this was the opposite. fix elastic#586

rshen91

This looks awesome! Tested the storybook locally on Chrome. Awesome addition of mock tests

nickofthyme

Code changes look good, just a few comments nothing blocking.

Verified storybook stories/playground in Chrome, Firefox and IE11.

Confirmed original issue is fixed with these changes.

One thing I still don't like is that rect annotations really only coexist with the Follow tooltip, any others the normal tooltip takes over. I think we should look into this with the tooltip redesign.

src/chart_types/xy_chart/annotations/line/dimensions.integration.test.ts

nickofthyme · 2020-04-28T13:38:23Z

src/chart_types/xy_chart/annotations/line/dimensions.ts

@@ -352,8 +330,7 @@ export function computeLineAnnotationDimensions(
      axisPosition,
      chartDimensions,
      lineColor,
-      xScaleOffset,


Why is this offset no longer used?

we compute the xScaleOffset internally, from the scale, we don't need anymore to pass it from the outside

src/chart_types/xy_chart/annotations/rect/dimensions.integration.test.ts

nickofthyme · 2020-04-28T13:49:47Z

src/chart_types/xy_chart/annotations/tooltip.ts

+  // allow picking up the last spec added as the top most or use it's zIndex value
+  const zIndexSortedSpecs = annotationSpecs
+    .slice()
+    .reverse()


Is reverse requried?

It's not required, I think we should think about that more broadly in the future.
I'm reversing the array because if I write a chart configuration like

<RectAnnotation dataValues={dataValuesGreen} id="rect3" style={{ fill: 'green' }} /> <RectAnnotation dataValues={dataValuesBlue} id="rect2" style={{ fill: 'blue' }} /> <RectAnnotation dataValues={dataValuesRed} id="rect1" style={{ fill: 'red' }} />

where I want to have the green on the back, and blue and red on top. My personal preference here is, when writing the spec, the first spec on the top will be rendered first, following a top-down => background->foreground order.

On the tooltip instead, I need to pick up the topmost rendered annotation (the red in our example), that is the last rendered. If I start traversing the annotationSpecs array in the "original" order and the green annotation is large enough to cover fully the chart, that the first annotation picked up by that algorithm is the green one.
Reversing it, I have the possibility to pick up the red as the first available annotation.

I know, this is for sure a naive approach, we can find a better way to handle that

src/chart_types/xy_chart/annotations/tooltip.ts

nickofthyme · 2020-04-28T13:51:56Z

src/chart_types/xy_chart/annotations/tooltip.ts

+  chartDimensions: Dimensions,
+): AnnotationTooltipState | null {
+  // allow picking up the last spec added as the top most or use it's zIndex value
+  const zIndexSortedSpecs = annotationSpecs


Nit: we don't really care how these are sorted in the variable name.

Suggested change

const zIndexSortedSpecs = annotationSpecs

const sortedSpecs = annotationSpecs

renamed here: 32dbf1f

src/chart_types/xy_chart/annotations/tooltip.ts

src/chart_types/xy_chart/utils/axis_utils.test.ts

src/mocks/store/store.ts

# [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 · 2020-04-28T21:57:05Z

🎉 This PR is included in version 19.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [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.

markov00 added bug Something isn't working :annotation Annotation (line, rect, text) related issue labels Apr 21, 2020

markov00 requested review from nickofthyme and rshen91 April 21, 2020 13:17

markov00 marked this pull request as draft April 21, 2020 16:06

markov00 added 3 commits April 24, 2020 16:16

fix: annotation extent cover fully the expressed range

ff44a8d

fix: align with new scale types

ff4120d

markov00 force-pushed the 2020_04_09-fix_annotations_extent branch from 1859c36 to ff4120d Compare April 24, 2020 16:45

markov00 added 4 commits April 24, 2020 19:08

fix: wrongly removed stripInternal config

4aa0882

refactor: remove unused code

76d22f4

Merge branch 'master' into 2020_04_09-fix_annotations_extent

e7e4954

test: fix VRTs

85917d8

markov00 marked this pull request as ready for review April 27, 2020 15:19

rshen91 approved these changes Apr 27, 2020

View reviewed changes

markov00 force-pushed the 2020_04_09-fix_annotations_extent branch from 58f78a8 to 85917d8 Compare April 28, 2020 12:30

nickofthyme approved these changes Apr 28, 2020

View reviewed changes

markov00 added 2 commits April 28, 2020 17:25

refactor: rename variable

32dbf1f

Merge branch 'master' into 2020_04_09-fix_annotations_extent

4f3e295

markov00 merged commit 43c5a59 into elastic:master Apr 28, 2020

markov00 deleted the 2020_04_09-fix_annotations_extent branch April 28, 2020 16:47

markov00 added the released Issue released publicly label Apr 28, 2020

markov00 mentioned this pull request Apr 29, 2020

Update dependency @elastic/charts to v19.1.2 elastic/kibana#64759

Merged

	const zIndexSortedSpecs = annotationSpecs
	const sortedSpecs = annotationSpecs

Conversation

markov00 commented Apr 21, 2020

Summary

Checklist

Uh oh!

codecov-io commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rshen91 left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickofthyme Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickofthyme Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickofthyme Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markov00 commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Apr 24, 2020 •

edited

Loading