fix(annotation): annotation rendering with no yDomain or groupId by rshen91 · Pull Request #842 · elastic/elastic-charts

rshen91 · 2020-10-01T18:41:21Z

Summary

Fixes #438 and #798 where <Annotations /> were not shown in cases where they should be.

The below story shows <Annotations /> when the y domain is [0, 0] or spans only one value (domain fit is true). In these cases, annotations will now take the chartDimension height:

The below story shows <Annotations /> rendering on the x axis if there is no provided group Id. If there is no group id provided and two y axes, the annotation will not still not show and this behavior is expected since the annotation will not know what y axis to mount onto:

Stories (Zero Domain and With Group Id) are added in Annotations -> Rect section in storybook.

Checklist

Delete any items that are not applicable to this PR.

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

codecov-commenter · 2020-10-02T18:45:08Z

Codecov Report

Merging #842 into master will increase coverage by 0.40%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master     #842      +/-   ##
==========================================
+ Coverage   70.01%   70.41%   +0.40%     
==========================================
  Files         319      334      +15     
  Lines       10474    10767     +293     
  Branches     2221     2266      +45     
==========================================
+ Hits         7333     7582     +249     
- Misses       3132     3170      +38     
- Partials        9       15       +6

Flag	Coverage Δ
#unittests	`69.91% <76.19%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
...hart_types/xy_chart/annotations/rect/dimensions.ts	`89.13% <75.00%> (-0.03%)`	⬇️
src/chart_types/xy_chart/annotations/utils.ts	`100.00% <100.00%> (ø)`
src/utils/d3-delaunay/index.ts	`48.69% <0.00%> (-1.49%)`	⬇️
src/mocks/series/series_identifiers.ts	`100.00% <0.00%> (ø)`
src/mocks/geometries.ts	`86.84% <0.00%> (ø)`
src/mocks/series/data.ts	`100.00% <0.00%> (ø)`
src/mocks/store/index.ts	`100.00% <0.00%> (ø)`
src/mocks/series/index.ts	`100.00% <0.00%> (ø)`
src/mocks/series/series.ts	`88.13% <0.00%> (ø)`
src/mocks/utils.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 4186962...2f51cdb. Read the comment docs.

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

codecov-io · 2020-10-06T21:02:09Z

Codecov Report

Merging #842 into master will increase coverage by 0.55%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #842      +/-   ##
==========================================
+ Coverage   70.01%   70.56%   +0.55%     
==========================================
  Files         321      336      +15     
  Lines       10508    10801     +293     
  Branches     2221     2182      -39     
==========================================
+ Hits         7357     7622     +265     
- Misses       3142     3164      +22     
- Partials        9       15       +6

Flag	Coverage Δ
#unittests	`70.06% <95.23%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
...es/xy_chart/state/selectors/compute_annotations.ts	`100.00% <ø> (ø)`
...hart_types/xy_chart/annotations/rect/dimensions.ts	`91.30% <95.00%> (+2.14%)`	⬆️
src/chart_types/xy_chart/annotations/utils.ts	`100.00% <100.00%> (ø)`
src/mocks/theme.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/specs.ts	`82.75% <0.00%> (ø)`
src/mocks/series/index.ts	`100.00% <0.00%> (ø)`
src/mocks/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/series/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/geometries.ts	`86.84% <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 66e1311...63a2df1. Read the comment docs.

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

nickofthyme

LGTM, I just made a few minor comments to fix before merging.

nickofthyme · 2020-10-12T15:58:07Z

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

    showOverlappingLabels: false,
    position: Position.Left,
    style,
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-call


Did you need this?

Fixed in 63a2df1

nickofthyme · 2020-10-12T15:58:11Z

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

    showOverlappingLabels: false,
    position: Position.Bottom,
    style,
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-call


Did you need this?

Nope 😅 ! 63a2df1

nickofthyme · 2020-10-12T15:58:23Z

src/chart_types/xy_chart/state/selectors/compute_annotations.ts

    computeSeriesGeometriesSelector,
    getAxisSpecsSelector,
    isHistogramModeEnabledSelector,
-    getAxisSpecsSelector,


nickofthyme · 2020-10-12T15:58:30Z

src/chart_types/xy_chart/state/utils/types.ts

-/** @internal */
-export interface BrushExtent {
-  minX: number;
-  minY: number;
-  maxX: number;
-  maxY: number;
-}
-


nickofthyme · 2020-10-12T16:00:18Z

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

+  it('annotation with group id should render with x0 and x1 values', () => {
+    const store = MockStore.default({ top: 0, left: 0, width: 20, height: 200 });
+    const settings = MockGlobalSpec.settingsNoMargins();
+    const annotation = MockAnnotationSpec.rect({
+      groupId: 'group1',
+      dataValues: [{ coordinates: { x0: 2, x1: 4 } }],
+    });
+    const bar = MockSeriesSpec.bar({
+      data: [
+        { x: 1, y: 0 },
+        { x: 2, y: 5 },
+        { x: 4, y: 10 },
+      ],
+    });
+    MockStore.addSpecs([settings, annotation, bar], store);
+    const expected = computeAnnotationDimensionsSelector(store.getState());
+    const [resultAnnotation] = expected.get('rect_annotation_1') ?? [];
+    expect(resultAnnotation).toMatchObject({
+      rect: { height: 200 },
+    });
+  });


You could make these more explicit that the expected value is the height being passed.

Suggested change

it('annotation with group id should render with x0 and x1 values', () => {

const store = MockStore.default({ top: 0, left: 0, width: 20, height: 200 });

const settings = MockGlobalSpec.settingsNoMargins();

const annotation = MockAnnotationSpec.rect({

groupId: 'group1',

dataValues: [{ coordinates: { x0: 2, x1: 4 } }],

});

const bar = MockSeriesSpec.bar({

data: [

{ x: 1, y: 0 },

{ x: 2, y: 5 },

{ x: 4, y: 10 },

],

});

MockStore.addSpecs([settings, annotation, bar], store);

const expected = computeAnnotationDimensionsSelector(store.getState());

const [resultAnnotation] = expected.get('rect_annotation_1') ?? [];

expect(resultAnnotation).toMatchObject({

rect: { height: 200 },

});

});

it('annotation with group id should render with x0 and x1 values', () => {

const height = 200;

const store = MockStore.default({ top: 0, left: 0, width: 20, height });

const settings = MockGlobalSpec.settingsNoMargins();

const annotation = MockAnnotationSpec.rect({

groupId: 'group1',

dataValues: [{ coordinates: { x0: 2, x1: 4 } }],

});

const bar = MockSeriesSpec.bar({

data: [

{ x: 1, y: 0 },

{ x: 2, y: 5 },

{ x: 4, y: 10 },

],

});

MockStore.addSpecs([settings, annotation, bar], store);

const expected = computeAnnotationDimensionsSelector(store.getState());

const [resultAnnotation] = expected.get('rect_annotation_1') ?? [];

expect(resultAnnotation).toMatchObject({

rect: { height },

});

});

Cool! Makes sense to make this explicit and clear that it's the same as the chartStore height - addressed in 63a2df1

# [24.0.0](v23.2.1...v24.0.0) (2020-10-19) ### Bug Fixes * **annotation:** annotation rendering with no yDomain or groupId ([#842](#842)) ([f173b49](f173b49)), closes [#438](#438) [#798](#798) ### Features * **bar_chart:** add Alignment offset to value labels ([#784](#784)) ([363aeb4](363aeb4)) * **bar_chart:** add shadow prop for value labels ([#785](#785)) ([9b29392](9b29392)) * **bar_chart:** scaled font size for value labels ([#789](#789)) ([3bdd1ee](3bdd1ee)), closes [#788](#788) * **heatmap:** allow fixed right margin ([#873](#873)) ([16cf73c](16cf73c)) ### BREAKING CHANGES * **bar_chart:** The `DisplayValueStyle` `fontSize` property can now express an upper and lower bound as size, used for the automatic scaling. * **bar_chart:** The `DisplayValueStyle` `fill` property can now express a border color and width, or let the library pick the best match based on contrast using the textInvertible parameter.

markov00 · 2020-10-19T13:49:45Z

🎉 This PR is included in version 24.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [24.0.0](elastic/elastic-charts@v23.2.1...v24.0.0) (2020-10-19) ### Bug Fixes * **annotation:** annotation rendering with no yDomain or groupId ([opensearch-project#842](elastic/elastic-charts#842)) ([6bad0d7](elastic/elastic-charts@6bad0d7)), closes [opensearch-project#438](elastic/elastic-charts#438) [opensearch-project#798](elastic/elastic-charts#798) ### Features * **bar_chart:** add Alignment offset to value labels ([opensearch-project#784](elastic/elastic-charts#784)) ([106d924](elastic/elastic-charts@106d924)) * **bar_chart:** add shadow prop for value labels ([opensearch-project#785](elastic/elastic-charts#785)) ([de95b44](elastic/elastic-charts@de95b44)) * **bar_chart:** scaled font size for value labels ([opensearch-project#789](elastic/elastic-charts#789)) ([8b74a9d](elastic/elastic-charts@8b74a9d)), closes [opensearch-project#788](elastic/elastic-charts#788) * **heatmap:** allow fixed right margin ([opensearch-project#873](elastic/elastic-charts#873)) ([dd34574](elastic/elastic-charts@dd34574)) ### BREAKING CHANGES * **bar_chart:** The `DisplayValueStyle` `fontSize` property can now express an upper and lower bound as size, used for the automatic scaling. * **bar_chart:** The `DisplayValueStyle` `fill` property can now express a border color and width, or let the library pick the best match based on contrast using the textInvertible parameter.

nickofthyme and others added 3 commits October 1, 2020 10:56

wip issue 1

00d6688

wip issue 2

e72611f

fix: fix tests

f2da526

rshen91 added bug Something isn't working :annotation Annotation (line, rect, text) related issue labels Oct 1, 2020

rshen91 added 3 commits October 1, 2020 15:24

WIP

6e7c2b7

test: add integration test and first stories

9d00de0

Merge branch 'master' into annotations-wip

2f51cdb

rshen91 added 2 commits October 3, 2020 08:26

feat: update stories

600773d

Merge remote-tracking branch 'upstream/master' into annotations-wip

831eb31

nickofthyme reviewed Oct 5, 2020

View reviewed changes

src/chart_types/xy_chart/annotations/utils.test.ts Outdated Show resolved Hide resolved

rshen91 added 8 commits October 5, 2020 08:28

fix: update vrts

2afb50c

Merge remote-tracking branch 'upstream/master' into annotations-wip

4b664bf

Merge remote-tracking branch 'upstream/master' into annotations-wip

b89b404

feat: fix stories for dimension logic

65d5023

test: add y minimizing ability on story

d05fc8b

test: add vrt annotations

2f3afcc

Merge remote-tracking branch 'upstream/master' into annotations-wip

78876d9

test: add integration test

76e05f8

rshen91 marked this pull request as ready for review October 6, 2020 21:15

rshen91 requested review from markov00 and nickofthyme October 6, 2020 21:16

nickofthyme reviewed Oct 6, 2020

View reviewed changes

src/chart_types/xy_chart/annotations/rect/dimensions.ts Outdated Show resolved Hide resolved

rshen91 changed the title ~~fix: annotation rendering with chartDimension.height~~ fix: annotation rendering with no yDomain Oct 7, 2020

fix: update if clause

9e90993

nickofthyme reviewed Oct 8, 2020

View reviewed changes

src/chart_types/xy_chart/annotations/rect/dimensions.ts Outdated Show resolved Hide resolved

fix: clean up tests and stories

19bc042

rshen91 requested a review from nickofthyme October 8, 2020 18:42

nickofthyme approved these changes Oct 12, 2020

View reviewed changes

refactor: update integration tests

63a2df1

rshen91 merged commit f173b49 into master Oct 12, 2020

rshen91 deleted the annotations-wip branch October 12, 2020 16:38

rshen91 changed the title ~~fix: annotation rendering with no yDomain~~ fix(annotation): annotation rendering with no yDomain or groupId Oct 12, 2020

markov00 added the released Issue released publicly label Oct 19, 2020

nickofthyme mentioned this pull request Nov 18, 2020

Rect Annotation should fallback to full height/width #798

Closed

This was referenced Jun 21, 2024

Defining a RectAnnotation as outside has not affect #2470

Closed

fix: outside rect annotation placement and group relations #2471

Merged

Conversation

rshen91 commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

codecov-commenter commented Oct 2, 2020

Codecov Report

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markov00 commented Oct 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rshen91 commented Oct 1, 2020 •

edited

Loading

codecov-io commented Oct 6, 2020 •

edited

Loading