Skip to content

fix(xy): use round ticks on linear bar charts#1435

Closed
markov00 wants to merge 9 commits intoelastic:masterfrom
markov00:2021_10_18-fix_linear_scale
Closed

fix(xy): use round ticks on linear bar charts#1435
markov00 wants to merge 9 commits intoelastic:masterfrom
markov00:2021_10_18-fix_linear_scale

Conversation

@markov00
Copy link
Copy Markdown
Collaborator

@markov00 markov00 commented Oct 19, 2021

Summary

This PR changes the way the X-axis is rendered in linear bar charts.
Instead of computing the ticks as

new Array(Math.floor((nicePaddedDomain[1] - nicePaddedDomain[0]) / minInterval) + 1)
            .fill(0)
            .map((_, i) => nicePaddedDomain[0] + i * minInterval);

I've converted this code to use the standard d3.scale.ticks instead.

This causes a lot of failures in the VRTs because a lot of bar chart uses the Linear scale on the x axis (that should be discouraged if not used in association with the histogram mode or with a controlled number of ticks)

The changes at the screenshot levels are also due to the reverting nature to ordinal scale in the X axis of bar chart and the subsequent removal of the legend extra parameter.

As discussed also with @monfera a bar chart can use the linear scale on the x scale, but its use slightly distorts the nature of the reading: in this case, the bar is only an adornment of a Y value and its width doesn't represent any more an interval in the x scale unless you use the histogram mode that correctly aligns the ticks at the beginning and end of the bar interval.

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)
  • The proper documentation and/or storybook story has been added or updated
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

On linear scale for bar charts, we manually compute the ticks, instead of using the tick function.
The commit change that behaviour using the right rounded ticks instead
Replace linear x scale with ordinal one when necessary.
Apply UTC timezone where using time scale
@markov00 markov00 added :data Data/series/scales related issue :xy Bar/Line/Area chart related labels Oct 20, 2021
@markov00 markov00 marked this pull request as ready for review October 20, 2021 20:58
@markov00 markov00 requested review from monfera, nickofthyme and rshen91 and removed request for monfera and rshen91 October 20, 2021 20:58
Copy link
Copy Markdown
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM - tested locally and saw no issues. Nice use of isXDomain hahaha

@monfera
Copy link
Copy Markdown
Contributor

monfera commented Oct 25, 2021

Good changes overall!

  1. Is the color change of the data ink intentional here?
    image

  2. Are we intentionally removing this detail from the legend here and in some other mocks?
    image

  3. I still have difficulty grasping the semantics of what looks like an ordinal bar chart, with too many tick lines compared to the bars. Should it do something that got added to the multilayer time axis today, which is, a constraint on no denser than the bin width? It may be in line with a tech demo intent, I'm still puzzled by such chart outputs, thinking, when would these be ever useful as well as not confusing?
    image
    image

  4. Should there be a tick for 40 on the horizontal axis? Or it's simply a mock not to be niced? I don't recall having a mock for niced X, but maybe it exists
    image

  5. Any specific reason for wanting to do a top horizontal axis in the past but not anymore? For example, here
    image

  6. [offtopic] We should have a storybook folder for "dear user, please don't make these kinds of charts" 😄
    image

@markov00
Copy link
Copy Markdown
Collaborator Author

Closing in favour of: #1645 #1646

@markov00 markov00 closed this Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:data Data/series/scales related issue :xy Bar/Line/Area chart related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants