fix(annotations): provide fallback for line annotation markers #1091

rshen91 · 2021-03-26T17:42:43Z

Summary

Fixes #1036
In the playground, if there is no SpecMarkerPosition this doesn't seem to lead to the same sort of bug where the marker is off the chart.

Added a story under test cases and test stories with rotations.

codecov-io · 2021-03-26T18:11:30Z

Codecov Report

Merging #1091 (c475411) into master (396b3d1) will increase coverage by 0.32%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
+ Coverage   71.82%   72.14%   +0.32%     
==========================================
  Files         381      397      +16     
  Lines       11917    12232     +315     
  Branches     2600     2628      +28     
==========================================
+ Hits         8559     8825     +266     
- Misses       3327     3368      +41     
- Partials       31       39       +8

Flag	Coverage Δ
unittests	`71.72% <72.72%> (-0.11%)`	⬇️

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

Impacted Files	Coverage Δ
...hart_types/xy_chart/annotations/line/dimensions.ts	`86.53% <72.72%> (ø)`
src/utils/d3-delaunay/index.ts	`50.30% <0.00%> (-1.49%)`	⬇️
src/mocks/theme.ts	`86.66% <0.00%> (ø)`
src/mocks/store/index.ts	`100.00% <0.00%> (ø)`
src/mocks/store/store.ts	`92.59% <0.00%> (ø)`
src/mocks/geometries.ts	`92.85% <0.00%> (ø)`
src/mocks/series/data.ts	`100.00% <0.00%> (ø)`
src/mocks/series/series.ts	`91.22% <0.00%> (ø)`
src/mocks/specs/specs.ts	`83.33% <0.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
... and 8 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 396b3d1...c475411. Read the comment docs.

…n-marker

markov00 · 2021-03-29T15:59:55Z

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

+  // catch the case of a chart with no axes nor specMarkerPosition for a vertical line annotation
+  if (!axisPosition && !specMarkerPosition && isXDomain) {
+    return Position.Bottom;
+  }


Can you please check the getDefaultMarkerPositionFromAxis function? I see that this also covers the case where the axisPosition is undefined but I don't know if we are considering all the cases.

Thank you! 2b4f5de

markov00

Left few comments: in particular, it needs to account for rotations.
Can you also please add a specific VRT that changes the rotation and checks the 4 rotations values screenshots?

markov00 · 2021-03-30T09:43:13Z

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

@@ -310,7 +309,7 @@ function getDefaultMarkerPositionFromAxis(
  if (axisPosition) {
    return axisPosition;
  }
-  if ((isXDomain && isHorizontal) || (!isXDomain && !isHorizontal)) {
+  if (!isXDomain && isHorizontal) {


This doesn't covers all the cases. Please check and fix this to cover the case where the chart is rotated 90,-90 degrees.

Thank you! Please see changes in e58089a 🙇‍♀️

markov00 · 2021-03-30T09:43:50Z

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

@@ -283,10 +283,9 @@ function getAnchorPosition(
  isXDomain: boolean,
  isChartHorizontal: boolean,
  specMarkerPosition?: Position,
-  axisPosition?: Position,
+  axisPosition?: Position | undefined,


this argument is already optional when using the ?

changed in e58089a thank you!

markov00 · 2021-03-30T09:45:06Z

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

@@ -297,7 +296,7 @@ function getAnchorPosition(

 function validateMarkerPosition(isXDomain: boolean, isHorizontal: boolean, position: Position): Position | undefined {
  if ((isXDomain && isHorizontal) || (!isXDomain && !isHorizontal)) {
-    return position === Position.Top || position === Position.Bottom ? position : undefined;
+    return position === Position.Top || position === Position.Bottom ? position : Position.Bottom;


this defaults to the dflPositionFromAxis if the configured specMarkerPosition doesn't pass this validation.
It's better to keep the defaults in a single place

Yep! Fixed in e58089a thank you

rshen91 · 2021-03-31T14:56:32Z

I was facing an issue in three stories (http://localhost:9001/?path=/story/bar-chart--functional-accessors, http://localhost:9001/?path=/story/annotations-rects--linear-bar-chart and https://elastic.github.io/elastic-charts/?path=/story/annotations-rects--ordinal-bar-chart) that were solved by this fix: storybookjs/storybook#6981 (comment) seen in commit e58089a

…n-marker

github-actions · 2021-03-31T15:11:30Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821200 run.

rshen91 · 2021-03-31T17:14:45Z

jenkins test this - accessibility test is flakey but has passed in 8c5e803

…n-marker

markov00

I haven't understood why we need to use that and what changed on master.
I've checked the currently deployed storybook and seems to work pretty well the text here.
Can you please run a rm -rf node_modules && yarn install to clean up the dependencies and checks if the problem persist without the marked dependency?

integration/tests/test_cases_stories.test.ts

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

rshen91 · 2021-04-02T15:37:56Z

I haven't understood why we need to use that and what changed on master.
I've checked the currently deployed storybook and seems to work pretty well the text here.
Can you please run a rm -rf node_modules && yarn install to clean up the dependencies and checks if the problem persist without the marked dependency?

You were right! Thanks! 0ba5d9b

markov00

All good for me! thanks for the changes

…n-marker

# [28.0.0](v27.0.0...v28.0.0) (2021-04-02) ### Bug Fixes * **annotations:** provide fallback for line annotation markers ([#1091](#1091)) ([0bd61f1](0bd61f1)) * **legend:** action sizing ui and focus states ([#1102](#1102)) ([3a76a2c](3a76a2c)) * **legend:** stop legend color picker dot twitching ([#1101](#1101)) ([c89b767](c89b767)) ### Code Refactoring * rename enum types to singular ([#1064](#1064)) ([396b3d1](396b3d1)), closes [#767](#767) ### BREAKING CHANGES * `AnnotationDomainTypes`, `AnnotationTypes`, `SeriesTypes`, `ChartTypes`, and `SpecTypes` are renamed to `AnnotationDomainType`, `AnnotationType`, `SeriesType`, `ChartType`, and `SpecType`

nickofthyme · 2021-04-02T18:58:21Z

🎉 This PR is included in version 28.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [28.0.0](elastic/elastic-charts@v27.0.0...v28.0.0) (2021-04-02) ### Bug Fixes * **annotations:** provide fallback for line annotation markers ([opensearch-project#1091](elastic/elastic-charts#1091)) ([d907c81](elastic/elastic-charts@d907c81)) * **legend:** action sizing ui and focus states ([opensearch-project#1102](elastic/elastic-charts#1102)) ([a58cc0a](elastic/elastic-charts@a58cc0a)) * **legend:** stop legend color picker dot twitching ([opensearch-project#1101](elastic/elastic-charts#1101)) ([f63bb3b](elastic/elastic-charts@f63bb3b)) ### Code Refactoring * rename enum types to singular ([opensearch-project#1064](elastic/elastic-charts#1064)) ([6e900e2](elastic/elastic-charts@6e900e2)), closes [opensearch-project#767](elastic/elastic-charts#767) ### BREAKING CHANGES * `AnnotationDomainTypes`, `AnnotationTypes`, `SeriesTypes`, `ChartTypes`, and `SpecTypes` are renamed to `AnnotationDomainType`, `AnnotationType`, `SeriesType`, `ChartType`, and `SpecType`

fix: catch edge case with vert line anno without axes nor specMarker

b495ef9

rshen91 added :annotation Annotation (line, rect, text) related issue wip work in progress labels Mar 26, 2021

rshen91 added 2 commits March 29, 2021 07:23

Merge remote-tracking branch 'upstream/master' into no-axis-annotatio…

90f7dec

…n-marker

test: add story

0496d01

rshen91 marked this pull request as ready for review March 29, 2021 15:09

rshen91 requested review from markov00, nickofthyme and monfera March 29, 2021 15:17

markov00 reviewed Mar 29, 2021

View reviewed changes

fix: refactor fix

2b4f5de

markov00 reviewed Mar 30, 2021

View reviewed changes

fix: adapt for rotations

e58089a

Merge remote-tracking branch 'upstream/master' into no-axis-annotatio…

4fd02b3

…n-marker

rshen91 removed the wip work in progress label Mar 31, 2021

test: add missing test

8159652

Merge remote-tracking branch 'upstream/master' into no-axis-annotatio…

8c5e803

…n-marker

rshen91 requested a review from markov00 April 1, 2021 15:30

Merge remote-tracking branch 'upstream/master' into no-axis-annotatio…

557cb8d

…n-marker

markov00 reviewed Apr 2, 2021

View reviewed changes

refactor: code review

0ba5d9b

test: removing flaky a11y test see PR 1099

52ee2df

rshen91 requested a review from markov00 April 2, 2021 16:36

markov00 approved these changes Apr 2, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into no-axis-annotatio…

c475411

…n-marker

rshen91 merged commit 0bd61f1 into elastic:master Apr 2, 2021

nickofthyme added the released Issue released publicly label Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(annotations): provide fallback for line annotation markers #1091

fix(annotations): provide fallback for line annotation markers #1091

rshen91 commented Mar 26, 2021 •

edited

Loading

codecov-io commented Mar 26, 2021 •

edited

Loading

markov00 Mar 29, 2021 •

edited

Loading

rshen91 Mar 29, 2021

markov00 left a comment

markov00 Mar 30, 2021

rshen91 Mar 31, 2021

markov00 Mar 30, 2021

rshen91 Mar 31, 2021

markov00 Mar 30, 2021

rshen91 Mar 31, 2021

rshen91 commented Mar 31, 2021

github-actions bot commented Mar 31, 2021

rshen91 commented Mar 31, 2021 •

edited

Loading

markov00 left a comment

rshen91 commented Apr 2, 2021

markov00 left a comment

nickofthyme commented Apr 2, 2021

fix(annotations): provide fallback for line annotation markers #1091

fix(annotations): provide fallback for line annotation markers #1091

Conversation

rshen91 commented Mar 26, 2021 • edited Loading

Summary

codecov-io commented Mar 26, 2021 • edited Loading

Codecov Report

markov00 Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

rshen91 Mar 29, 2021

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 Mar 30, 2021

Choose a reason for hiding this comment

rshen91 Mar 31, 2021

Choose a reason for hiding this comment

markov00 Mar 30, 2021

Choose a reason for hiding this comment

rshen91 Mar 31, 2021

Choose a reason for hiding this comment

markov00 Mar 30, 2021

Choose a reason for hiding this comment

rshen91 Mar 31, 2021

Choose a reason for hiding this comment

rshen91 commented Mar 31, 2021

github-actions bot commented Mar 31, 2021

rshen91 commented Mar 31, 2021 • edited Loading

markov00 left a comment

Choose a reason for hiding this comment

rshen91 commented Apr 2, 2021

markov00 left a comment

Choose a reason for hiding this comment

nickofthyme commented Apr 2, 2021

rshen91 commented Mar 26, 2021 •

edited

Loading

codecov-io commented Mar 26, 2021 •

edited

Loading

markov00 Mar 29, 2021 •

edited

Loading

rshen91 commented Mar 31, 2021 •

edited

Loading