Skip to content

feat(axes): option to fit domain to list of annotation SpecIds#1641

Merged
nickofthyme merged 17 commits intoelastic:masterfrom
nickofthyme:anno-fit-domain
Apr 14, 2022
Merged

feat(axes): option to fit domain to list of annotation SpecIds#1641
nickofthyme merged 17 commits intoelastic:masterfrom
nickofthyme:anno-fit-domain

Conversation

@nickofthyme
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme commented Apr 6, 2022 •
edited
Loading

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 nickofthyme requested a review from markov00 April 6, 2022 17:31
@nickofthyme
Copy link
Copy Markdown
Collaborator Author

nickofthyme commented Apr 7, 2022 •
edited
Loading

@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
Copy Markdown
Collaborator

markov00 commented Apr 11, 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.

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

markov00
markov00 reviewed Apr 11, 2022
View reviewed changes
@nickofthyme nickofthyme requested a review from markov00 April 12, 2022 18:47
markov00
markov00 approved these changes Apr 14, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@markov00 markov00 left a comment •
edited
Loading

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
@semantic-release-bot
chore(release): 46.0.0 [skip ci]
69e7765 
# [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

Reviewers

@markov00 markov00 markov00 approved these changes

Assignees

No one assigned

Labels

:axis Axis related issue :xy Bar/Line/Area chart related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Axis domain fit option to include also annotation's data

2 participants

@nickofthyme @markov00