feat(rect_annotation): add RectAnnotation type#180
feat(rect_annotation): add RectAnnotation type#180emmacunningham merged 48 commits intoelastic:masterfrom
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
jenkins test this |
markov00
left a comment
There was a problem hiding this comment.
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
|
@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. |
|
jenkins test this |
markov00
left a comment
There was a problem hiding this comment.
I've added few comments that I think we should address before merging. Everything else is amazing, Thanks!
src/specs/rect_annotation.tsx
Outdated
| static defaultProps: Partial<RectAnnotationProps> = { | ||
| groupId: getGroupId('__global__'), | ||
| annotationType: 'rectangle', | ||
| zIndex: 0, |
There was a problem hiding this comment.
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)
src/specs/line_annotation.tsx
Outdated
| style: DEFAULT_ANNOTATION_LINE_STYLE, | ||
| hideLines: false, | ||
| hideTooltips: false, | ||
| zIndex: 0, |
There was a problem hiding this comment.
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
src/lib/series/specs.ts
Outdated
| /** Toggles tooltip annotation visibility */ | ||
| hideTooltips?: boolean; | ||
| /** z-index of the annotation relative to other elements in the chart | ||
| * @default 0 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))
|
🎉 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))

Summary
close #105
This PR introduces the functionality to add a
RectAnnotationspec which will draw a rectangular annotation in a chart. It also refactors the Annotation specs such that bothLineAnnotationSpecandRectAnnotationSpecextend the sameBaseAnnotationSpecfor shared properties:BaseAnnotationSpecspecifies:The
RectAnnotationSpecis defined as follows:where each annotation dataValue (
RectAnnotationDatum) has the shape: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:
The annotation may also be styled by passing a custom style, which allows customization of
stroke,strokeWidth,opacity, andfill.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
zIndexwhich controls the relative z positioning of the annotations:Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.