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(xy)!: change timeAxisLayerCount the default from 0 to 2 #2582

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

walterra
Copy link

@walterra walterra commented Jan 13, 2025

Summary

timeAxisLayerCount: 2 is now the default to render multiple layers for time based axes. This feature was originally added in 2021 and now just a small set of consumers are using the legacy time axis.

BREAKING CHANGE

The timeAxisLayerCount options now defaults to 2 instead of 0.

Details

  • This includes some tricky changes: Because timeAxisLayerCount defaulted to 0, code in some places assumed that a change to 2 was only done by users when the x axis was then time based. Thus changing the default to 2 changed the calculated padding for all axis types.
  • On top of that this surfaced a limitation of this setting: When timeAxisLayerCount is set to 2, there was no check for axes if they were actually time based. This means this also affected vertical axes so the left and right margin would change. I think that's the reason why we see so many baseline changes in the playwright e2e image assertions.

Issues

This PR is part of elastic/kibana#193683

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

@walterra walterra changed the title make timeAxisLayerCount: 2 the default feat(xy): make timeAxisLayerCount: 2 the default Jan 13, 2025
@walterra walterra self-assigned this Jan 13, 2025
@walterra walterra added :axis Axis related issue :xy Bar/Line/Area chart related labels Jan 13, 2025
@nickofthyme
Copy link
Collaborator

nickofthyme commented Jan 13, 2025

@walterra FYI, to update any playwright snapshots you can comment on the PR with..

buildkite update screenshots

This will run the ci in update mode and push a commit to your branch with the updated e2e screenshots, if any. The reason we need to run this in the CI is due to slight differences in the browser binaries between MacOS and linux ci agents. So to avoid this we only store screenshots generated from ci.

And to just trigger a re-run of the ci you can comment...

buildkite test this

@walterra walterra force-pushed the remove-legacy-time-axis-support branch from 6b22f3a to fd7ac96 Compare January 14, 2025 15:57
@walterra walterra changed the title feat(xy): make timeAxisLayerCount: 2 the default fix!(xy): change timeAxisLayerCount the default from 0 to 2 Jan 16, 2025
@walterra walterra changed the title fix!(xy): change timeAxisLayerCount the default from 0 to 2 feat!(xy): change timeAxisLayerCount the default from 0 to 2 Jan 17, 2025
@walterra walterra changed the title feat!(xy): change timeAxisLayerCount the default from 0 to 2 feat(xy)!: change timeAxisLayerCount the default from 0 to 2 Jan 17, 2025
@walterra walterra added kibana cross issue Has a Kibana issue counterpart and removed kibana cross issue Has a Kibana issue counterpart labels Jan 20, 2025
@@ -81,24 +81,7 @@ export const Example: ChartsStory = (_, { title, description }) => {
},
Copy link
Author

Choose a reason for hiding this comment

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

This still causes a baseline change, right padding in the lowest chart:

CleanShot 2025-01-20 at 15 24 09@2x

Copy link
Author

Choose a reason for hiding this comment

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

This reason for this regression: We now have to check for both timeAxisLayerCount > 0 and if the scale type is time, however, the lowest chart uses ordinal.

Copy link
Author

Choose a reason for hiding this comment

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

If I change the xScaleType of the BarSeries of the last chart to ScaleType.Time it looks correct:

CleanShot 2025-01-20 at 16 01 07@2x

@@ -132,7 +132,7 @@ Show duplicated ticks

### `timeAxisLayerCount`
- Type: <code>timeAxisLayerCount: number</code>
- Default: `0`
- Default: `2`
Copy link
Author

@walterra walterra Jan 20, 2025

Choose a reason for hiding this comment

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

Posting this here for reference to start a thread, because the affected example file has not been touched.

There's a baseline change for stories with custom markers. Previously the axis ticks and markers would render overlapping, now they render with additional offset. Is the new version ok? I'm not sure the original overlapping was intentional or a bug?

https://ech-e2e-ci--pr-2582-n5t2yfqp.web.app/e2e-report/#?testId=9cb55a84df16c692f1c3-042343d23333860efb20

CleanShot 2025-01-20 at 16 44 45@2x

CleanShot 2025-01-20 at 16 47 18@2x

@walterra walterra force-pushed the remove-legacy-time-axis-support branch from 3a7832f to abddedc Compare January 21, 2025 15:59
@elastic-datavis
Copy link
Contributor

✅ Successful Deployment (build#4404) - abddedc

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.

2 participants