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

refactor(axes): replace integersOnly with maximumFractionDigits #2305

Merged
merged 6 commits into from
Jan 26, 2024

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Jan 17, 2024

Summary

Adds a way to round axes tick values prior to formatting the numerical values into a string.

⚠️ The Axis.integersOnly prop is now deprecated and replace with maximumFractionDigits ⚠️

Usages

Below shows, toggling maximumFractionDigits between 0 (integers only) and 1 (single decimal or tenths only)

Screen Recording 2024-01-16 at 07 44 46 PM

Below shows, toggling maximumFractionDigits between 0 (integers only), 1 (single decimal or tenths only) and 2 (double decimal or hundredths only)

Screen Recording 2024-01-16 at 08 09 41 PM

Details

The original logic to add the integersOnly only allowed rounding of the numerical tick value in the case of an integer. This PR expands on that by allowing the chart consumer to set the max number of fractional digits (aka decimals) in a numerical value. This is different than formatting in order to show a value to a max fractional digits, as this can render deceptive ticks and their respective placements, see linked issue.

Drawbacks

This is not an active assignment of ticks given the rounded constraint but rather a passive removal of ticks that do not abide by the constraint. Currently d3-scale does not support asking for ticks in this manner. Thus there are drawbacks to this approach, namely those listed below:

  • Nicing end ticks can be removed - Given the range extents, there are some cases where nicing a domain with a restrictive maximumFractionDigits value with result in one or both of the end ticks being removed. See the second gif above as an example of this.
  • Tick count is more variable - Say we set maximumFractionDigits to 1 and out data domain is between 0 and 1. Then let's say we desire ~10 ticks, that would be a tick every 0.1 unit. Here we would keep all the ticks as it would maintain all 0.X values. But what if we specify ~4 ticks, d3 would likely give us ticks at every 0.25 unit, in this case all ticks but the 0 and 1 would be removed as then are 0.XX not 0.X. I don't see a way to fix this without iterating a few times, or with custom tick logic.
  • Less density of ticks - This is hard to avoid as the very nature of rounding would limit the possible ticks available to show.

Issues

Closes #2220

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
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with light and dark themes

@nickofthyme nickofthyme added enhancement New feature or request kibana cross issue Has a Kibana issue counterpart :axis Axis related issue :xy Bar/Line/Area chart related :Lens Kibana Lens related issue labels Jan 17, 2024
@nickofthyme nickofthyme marked this pull request as ready for review January 23, 2024 22:27
@nickofthyme nickofthyme added ci:skip Skip all build checks and removed ci:skip Skip all build checks labels Jan 23, 2024
@nickofthyme nickofthyme reopened this Jan 23, 2024
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

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.

Screenshot 2024-01-24 at 15 38 57

This doesn't look correct to me, with 0 digit the 1 should not be there, the nice scale shows 0.7 but when changing to 0 fractional digits the 0.7 transforms to 1

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.

Let's merge this and fix the other issue in a subsequent PR, thanks

@nickofthyme nickofthyme merged commit 31b33c3 into elastic:main Jan 26, 2024
13 checks passed
@nickofthyme nickofthyme deleted the max-frac-digit branch January 26, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue enhancement New feature or request kibana cross issue Has a Kibana issue counterpart :Lens Kibana Lens related issue :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Axis.integersOnly option with maximumFractionDigits
2 participants