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(axes): option to fit domain to list of annotation SpecIds #1641

Merged
merged 17 commits into from
Apr 14, 2022

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Apr 6, 2022

Summary

Adds the AxisSpec.domain.includeDataFromIds option to YDomainBase. This optional allows specifying a series of SpecIds to include into the domain calculation, respective of their groupId. Currently, only LineAnnotation and RectAnnotation specs are supported, everything else is already included in the domain automatically. Setting domain.max or domain.min will override this functionality.

Apr-06-2022 11-09-54

See pr demo story here

Issues

fix #1411

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated

@nickofthyme nickofthyme added :axis Axis related issue :xy Bar/Line/Area chart related labels Apr 6, 2022
@nickofthyme
Copy link
Collaborator Author

nickofthyme commented Apr 7, 2022

@markov00 I added the rect annotation finite values to the domain. The one question I have is when the domain min is controlled by y0 or the max by y1, the rect annotation covers the full area of the chart with no real indication that it's an annotation without the some type of padding. See example.

Screen Recording 2022-04-07 at 05 38 48 PM

I wonder if we should filter the y0 rect annotation values that are below the computed min and y1 values above the computed max.

@nickofthyme nickofthyme marked this pull request as ready for review April 7, 2022 22:46
@markov00
Copy link
Member

@markov00 I added the rect annotation finite values to the domain. The one question I have is when the domain min is controlled by y0 or the max by y1, the rect annotation covers the full area of the chart with no real indication that it's an annotation without the some type of padding. See example.

I wonder if we should filter the y0 rect annotation values that are below the computed min and y1 values above the computed max.

I don't see this as a problem. If I'm adding an annotation with a data domain that is bigger then the current chart data domain, and I configured my chart to include the annotation domain into the data once, I don't see this as an issue: a rectangle area that covers entirely the chart works fine for me, it doesn't need anything special

@nickofthyme nickofthyme requested a review from markov00 April 12, 2022 18:47
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, everything works like perfectly, I have also tested with the yNice prop enabled and it follows that correctly.

Please update the PR description with the changes you have done on the naming

@nickofthyme nickofthyme changed the title feat(axes): option to fit domain to line annotations values feat(axes): option to fit domain to list of annotation SpecIds Apr 14, 2022
@nickofthyme nickofthyme merged commit 220350d into elastic:master Apr 14, 2022
@nickofthyme nickofthyme deleted the anno-fit-domain branch April 14, 2022 13:13
nickofthyme pushed a commit that referenced this pull request Apr 14, 2022
# [46.0.0](v45.1.1...v46.0.0) (2022-04-14)

### Bug Fixes

* **axis:** ticks generation for linear scale on bar charts ([#1645](#1645)) ([65d0e7d](65d0e7d))
* **axis:** use correct desired tick count based on axis type ([#1646](#1646)) ([512a6cd](512a6cd))
* **deps:** update dependency @elastic/eui to v53 ([#1639](#1639)) ([34bf325](34bf325))
* **deps:** update dependency @elastic/eui to v54 ([#1642](#1642)) ([6eaca0a](6eaca0a))

### Features

* **axes:** option to fit domain to list of annotation `SpecIds` ([#1641](#1641)) ([220350d](220350d))
* **goal:** auto generated linear ticks ([#1637](#1637)) ([5437d8e](5437d8e))
* **legend:** expose sorting function ([#1644](#1644)) ([128114c](128114c))

### BREAKING CHANGES

* **goal:** goal chart now requires domain min and max to be defined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Axis domain fit option to include also annotation's data
2 participants