Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(annotations): render line annotations via LineAnnotation spec #126

Merged
merged 53 commits into from
Apr 4, 2019

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented Mar 27, 2019

Summary

close #104

This PR introduces a LineAnnotation spec so the user can specify what annotations on a chart they want.

Since this is the first of the Annotation features, this also introduces a bit of scaffolding for the annotations overall. This implementation allows a user to define a set of annotations by defining one of three specs (LineAnnotation, RectangleAnnotation, and TextAnnotation).

Within a LineAnnotation spec a user may define:

  /** The id of the annotation */
  annotationId: AnnotationId;
  /** Annotation type: line, rectangle, text */
  annotationType: AnnotationType;
  /** The ID of the axis group, generated via getGroupId method */
  groupId: GroupId; // defaults to __global__; needed for yDomain position
  /** Annotation domain type: XDomain or YDomain */
  domainType: AnnotationDomainType;
  /** Data values defined with value, details, and header */
  dataValues: AnnotationDatum[];
  /** Custom line styles */
  lineStyle?: Partial<AnnotationLineStyle>;
  /** Custom marker */
  marker?: JSX.Element;

dataValues are defined in terms of data relevant to a scale within the chart. If the domainType is xDomain, then the annotation points will be scaled relative to the xDomain scale. If the domainType is yDomain, then the annotation points will be scaled relative to a yDomain scale; we get the relevant scale by lookup up against this.yScales with the groupId defined on the spec.

x_continuous_annotation
ordinal_annotation
time_series_annotation

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

@emmacunningham emmacunningham added the wip work in progress label Mar 27, 2019
@codecov-io
Copy link

codecov-io commented Mar 27, 2019

Codecov Report

Merging #126 into master will increase coverage by 0.83%.
The diff coverage is 99.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   92.84%   93.67%   +0.83%     
==========================================
  Files          31       32       +1     
  Lines        1481     1660     +179     
  Branches      163      194      +31     
==========================================
+ Hits         1375     1555     +180     
+ Misses         92       90       -2     
- Partials       14       15       +1
Impacted Files Coverage Δ
src/lib/themes/theme.ts 100% <100%> (ø) ⬆️
src/lib/series/specs.ts 100% <100%> (ø) ⬆️
src/lib/utils/ids.ts 100% <100%> (ø) ⬆️
src/state/chart_state.ts 95.69% <100%> (+0.17%) ⬆️
src/state/annotation_utils.ts 99.32% <99.32%> (ø)
src/state/utils.ts 90.64% <0%> (+1.16%) ⬆️

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 56b173d...895bc6e. Read the comment docs.

@emmacunningham emmacunningham added :annotation Annotation (line, rect, text) related issue and removed wip work in progress labels Apr 3, 2019
@markov00 markov00 mentioned this pull request Apr 3, 2019
93 tasks
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and works fine now.
I've found just few minor things:

  • on scale continuous, if you add an annotation with an empty string, it will be configured as an annotation on 0, this because d3-scale coerce values...
  • I think you can keep the LineAnnotation style prop as if was before: Partial and optional, as you will always merge it on chartState when adding the annotation spec.

Beside that last point, I think it's good to merge

@markov00 markov00 added the enhancement New feature or request label Apr 4, 2019
@emmacunningham
Copy link
Contributor Author

Made LineAnnotationSpec.style partial and optional here 0e588eb (and added story for styling ff6b70f)

We explicitly check for the empty string now to prevent the error when d3.scale('') returns 0. (47406ae and 1a2e08f)

empty_string

@emmacunningham emmacunningham merged commit 98ff170 into elastic:master Apr 4, 2019
markov00 pushed a commit that referenced this pull request Apr 4, 2019
# [3.6.0](v3.5.1...v3.6.0) (2019-04-04)

### Features

* **annotations:** render line annotations via LineAnnotation spec ([#126](#126)) ([98ff170](98ff170))
@markov00
Copy link
Member

markov00 commented Apr 4, 2019

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 4, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [3.6.0](elastic/elastic-charts@v3.5.1...v3.6.0) (2019-04-04)

### Features

* **annotations:** render line annotations via LineAnnotation spec ([opensearch-project#126](elastic/elastic-charts#126)) ([ac9313f](elastic/elastic-charts@ac9313f))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:annotation Annotation (line, rect, text) related issue enhancement New feature or request released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add annotation line, tooltip, and marker
3 participants