feat(rect_annotation): add RectAnnotation type #180

emmacunningham · 2019-04-20T01:30:16Z

Summary

close #105

This PR introduces the functionality to add a RectAnnotation spec which will draw a rectangular annotation in a chart. It also refactors the Annotation specs such that both LineAnnotationSpec and RectAnnotationSpec extend the same BaseAnnotationSpec for shared properties:

BaseAnnotationSpec specifies:

export interface BaseAnnotationSpec {
  /** The id of the annotation */
  annotationId: AnnotationId;
  /** Annotation type: line, rectangle, text */
  annotationType: AnnotationType;
  /** The ID of the axis group, generated via getGroupId method
   * @default __global__
   */
  groupId: GroupId; // defaults to __global__; needed for yDomain position
  /** Data values defined with coordinates and details */
  dataValues: AnnotationDatum[];
  /** Custom annotation style */
  style?: Partial<AnnotationStyle>;
  /** Toggles tooltip annotation visibility */
  hideTooltips?: boolean;
  /** z-index of the annotation relative to other elements in the chart
   * @default 0
   */
  zIndex?: number;
}

The RectAnnotationSpec is defined as follows:

export interface RectAnnotationSpec {
  annotationType: 'rectangle';
  /** Custom rendering function for tooltip */
  renderTooltip?: (
      position: { transform: string; top: number; left: number; }, 
      details?: string
    ) => JSX.Element;
  /** Data values defined with coordinates and details */
  dataValues: RectAnnotationDatum[];
  /** Custom annotation style */
  style?: Partial<RectAnnotationStyle>;
}

where each annotation dataValue (RectAnnotationDatum) has the shape:

export interface RectAnnotationDatum {
  coordinates: {
    x0?: any;
    x1?: any;
    y0?: any;
    y1?: any;
  };
  details?: string;
}

We validate & coerce the coordinate values such that if no valid coordinates are found, the annotation will be skipped & not render; so long as one of the coordinates is defined, we will draw the annotation by coercing the undefined coordinates to the chart extents such that:

x0	x1	y0	y1	render implications
undefined	undefined	undefined	undefined	no rendered annotation
defined	undefined (coerced to x domain start)	undefined (coerced to y domain end)	undefined (coerced to y domain start)	rect will be drawn from xDomain start to x1 with full chart height
undefined (coerced to x domain end)	defined	undefined (coerced to y domain end)	undefined (coerced to y domain start)	rect will be drawn from x1 to x domain end with full chart height
undefined (coerced to x domain end)	undefined (coerced to x domain start)	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 end)	undefined (coerced to x domain start)	undefined (coerced to y domain end)	defined	rect will be drawn from y1 to y domain end with full chart width

The annotation may also be styled by passing a custom style, which allows customization of stroke, strokeWidth, opacity, and fill.

In the case where an annotation's hover tooltip is visible where there is also a chart element hover tooltip, precedence will go to the annotation tooltip unless the chart element hover has a specific element highlighted in which case the chart element tooltip will be visible instead. (This matches the behavior of the tooltip on the annotations in Discover)

Finally, we added a new prop zIndex which controls the relative z positioning of the annotations:

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

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

codecov-io · 2019-04-20T01:40:34Z

Codecov Report

Merging #180 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master    #180     +/-   ##
========================================
+ Coverage   97.19%   97.4%   +0.2%     
========================================
  Files          35      35             
  Lines        1962    2120    +158     
  Branches      285     315     +30     
========================================
+ Hits         1907    2065    +158     
  Misses         48      48             
  Partials        7       7

Impacted Files	Coverage Δ
src/lib/themes/theme.ts	`100% <100%> (ø)`	⬆️
src/lib/series/specs.ts	`100% <100%> (ø)`	⬆️
src/state/chart_state.ts	`96.41% <100%> (+0.14%)`	⬆️
src/state/annotation_utils.ts	`100% <100%> (ø)`	⬆️

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 0897ff1...a56c2af. Read the comment docs.

emmacunningham · 2019-04-22T20:17:34Z

jenkins test this

markov00

Code LGTM. Tested locally. I've a minor/major feature that I think we need to add before release this:
I think we should include a prop to specify if the annotation is on the foreground or on the background.
For example, on the current vislib the gray band that specify partial buckets are behind the bars, but the line annotations are on the foreground.
We can introduce here the concept of z-index, and use this to render annotation above or below the chart elements.
We can than move this concept also to chart elements

emmacunningham · 2019-05-01T16:34:15Z

@markov00 Added the zIndex prop to BaseAnnotationSpec, which will now control the ordering of how the elements. Because of how react-konva doesn't handle zIndex (and instead relies on rendering order for this), this required a change in the implementation of the annotations; they now are rendered in the same layer as the chart elements. I will leave the remaining work of ordering the chart elements to a separate PR as that will require grouping the chart elements by series & rendering the groups per zIndex on the series.

emmacunningham · 2019-05-02T16:56:43Z

jenkins test this

markov00

I've added few comments that I think we should address before merging. Everything else is amazing, Thanks!

markov00 · 2019-05-03T10:17:32Z

src/specs/rect_annotation.tsx

@@ -10,6 +10,7 @@ export class RectAnnotationSpecComponent extends PureComponent<RectAnnotationPro
  static defaultProps: Partial<RectAnnotationProps> = {
    groupId: getGroupId('__global__'),
    annotationType: 'rectangle',
+    zIndex: 0,


I think it's better to move the rect annotation on the background as default with a zIndex of -1 or -10 (so that on our current Kibana use cases we don't have to specify the zIndex for each RectAnnotation)

👍 added here d9aa483

markov00 · 2019-05-03T10:18:31Z

src/specs/line_annotation.tsx

@@ -14,6 +14,7 @@ export class LineAnnotationSpecComponent extends PureComponent<LineAnnotationPro
    style: DEFAULT_ANNOTATION_LINE_STYLE,
    hideLines: false,
    hideTooltips: false,
+    zIndex: 0,


We should already specify a default zIndex of 1 or 10 for lines so that our current use-cases are already satisfied bu our defaults

added default here d9aa483

markov00 · 2019-05-03T10:20:53Z

src/lib/series/specs.ts

@@ -255,6 +255,10 @@ export interface BaseAnnotationSpec {
  style?: Partial<AnnotationStyle>;
  /** Toggles tooltip annotation visibility */
  hideTooltips?: boolean;
+  /** z-index of the annotation relative to other elements in the chart
+   * @default 0


I think we should specify the default in a different way, as per my comment below we should use two different defaults, one for Line and one for Rect

have added a comment to the individual specs for RectAnnotation and LineAnnotation the different default values d9aa483

# [4.2.0](v4.1.0...v4.2.0) (2019-05-06) ### Features * **rect_annotation:** add RectAnnotation type ([#180](#180)) ([b339318](b339318))

markov00 · 2019-05-06T03:03:41Z

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [4.2.0](elastic/elastic-charts@v4.1.0...v4.2.0) (2019-05-06) ### Features * **rect_annotation:** add RectAnnotation type ([opensearch-project#180](elastic/elastic-charts#180)) ([ec6c720](elastic/elastic-charts@ec6c720))

emmacunningham added 19 commits April 16, 2019 11:12

refactor: use type guards for annotation types

affd225

docs(annotations): add scaffold for rect annotation

8cbd978

refactor(annotations): change AnnotationDatum to LineAnnotationDatum

7fa7a09

feat(rect_annotation): add rect_annotation story with full spec

526bf21

refactor: rename line-only annotation component

b3d8d91

refactor: add scaffold to render rect_annotation

c09dc42

feat(rect_annotation): display rect annotation on unrotated chart

651a5d3

feat(rect_annotation): adjust rectDimensions based on chartRotation

4c33f50

refactor(annotation): add annotation type to tooltip

7f5461c

feat(rect_annotation): show tooltip on hover

a68593e

feat(rect_annotation): position tooltip relative to side of highlight

4ff0d46

feat(rect_annotation): position based on cursor relative to chart

de4995e

feat(rect_annotation): position tooltip correctly dependent on rotation

d0da43b

refactor(rect_annotation): remove unnecessary axesPositions parameter

dd57ba6

feat(rect_annotation): add marker to tooltip

bc87583

refactor(rect_annotation): remove markerDimensions from props

1962457

feat(annotation): add clippings

03cee1c

feat(annotation): prevent annotation & chart element tooltip conflicts

254baf2

test(rect_annotation): add test coverage for rect_annotation utils

0e5cc45

emmacunningham added enhancement New feature or request wip work in progress :annotation Annotation (line, rect, text) related issue labels Apr 20, 2019

chore: merge master into feat/annotation-rect

c8641e7

emmacunningham added 2 commits April 22, 2019 08:41

test: add test for merging annotation rect style

1b0242b

test: add test for annotationTooltipState

e292c0b

emmacunningham closed this Apr 22, 2019

emmacunningham reopened this Apr 22, 2019

emmacunningham removed the wip work in progress label Apr 22, 2019

chore: merge master into feat/annotation-rect

1358256

emmacunningham requested review from markov00 and removed request for markov00 April 29, 2019 15:24

markov00 reviewed Apr 30, 2019

View reviewed changes

emmacunningham added 8 commits April 30, 2019 09:43

refactor(annotations): move annotations into series render layer

57bbe17

feat: render chart elements relative to zIndex

4757ee9

fix(rect_annotation): fix offset for band scale

ef35e2a

fix(rect_annotation): fix rotation issues

380304c

fix(rect_annotation): fix tooltip issues on rotation

90e359c

fix(line_annotation): fix issues with rotation

9c4e3b4

fix(reactive_chart): add key for elements

4cc3991

fix(line_annotation): fix marker hover state

a9d9546

emmacunningham requested a review from markov00 May 1, 2019 16:34

emmacunningham added 2 commits May 1, 2019 09:37

feat(annotations): set default zIndex to 0

73c33cd

chore: merge master into feat/annotation-rect

921468f

markov00 reviewed May 3, 2019

View reviewed changes

emmacunningham added 6 commits May 3, 2019 16:46

feat(annotations): set zIndex per spec

d9aa483

chore: merge master into feat/annotation-rect

1324bef

test(annotattion): fix isWithinLineBounds ttests

1a41a7f

test(annotation_utils): add test for rotation helpers

83be481

chore: merge master into feat/annotation-rect

182ecd7

test(annotation_utils): fill in coverage for changed rotation logic

a56c2af

emmacunningham merged commit b339318 into elastic:master May 6, 2019

markov00 pushed a commit that referenced this pull request May 6, 2019

chore(release): 4.2.0 [skip ci]

65f257e

# [4.2.0](v4.1.0...v4.2.0) (2019-05-06) ### Features * **rect_annotation:** add RectAnnotation type ([#180](#180)) ([b339318](b339318))

markov00 added the released Issue released publicly label May 6, 2019

markov00 mentioned this pull request Aug 22, 2019

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rect_annotation): add RectAnnotation type #180

feat(rect_annotation): add RectAnnotation type #180

emmacunningham commented Apr 20, 2019 •

edited

Loading

codecov-io commented Apr 20, 2019 •

edited

Loading

emmacunningham commented Apr 22, 2019

markov00 left a comment

emmacunningham commented May 1, 2019

emmacunningham commented May 2, 2019

markov00 left a comment

markov00 May 3, 2019

emmacunningham May 3, 2019

markov00 May 3, 2019

emmacunningham May 3, 2019

markov00 May 3, 2019

emmacunningham May 3, 2019

markov00 commented May 6, 2019

feat(rect_annotation): add RectAnnotation type #180

feat(rect_annotation): add RectAnnotation type #180

Conversation

emmacunningham commented Apr 20, 2019 • edited Loading

Summary

Checklist

codecov-io commented Apr 20, 2019 • edited Loading

Codecov Report

emmacunningham commented Apr 22, 2019

markov00 left a comment

Choose a reason for hiding this comment

emmacunningham commented May 1, 2019

emmacunningham commented May 2, 2019

markov00 left a comment

Choose a reason for hiding this comment

markov00 May 3, 2019

Choose a reason for hiding this comment

emmacunningham May 3, 2019

Choose a reason for hiding this comment

markov00 May 3, 2019

Choose a reason for hiding this comment

emmacunningham May 3, 2019

Choose a reason for hiding this comment

markov00 May 3, 2019

Choose a reason for hiding this comment

emmacunningham May 3, 2019

Choose a reason for hiding this comment

markov00 commented May 6, 2019

emmacunningham commented Apr 20, 2019 •

edited

Loading

codecov-io commented Apr 20, 2019 •

edited

Loading