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

fix(xy): switch default timezone to local #1544

Merged

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jan 12, 2022

Summary

The default timezone used when no timezone prop is provided is now aligned to the browser's local timezone.

BREAKING CHANGE

The time axis labels of a time-series chart configured without the timeZone prop are now rendering the labels with the local browser timezone instead of UTC.

Details

The PR #1383 correctly fixed our internal logic that uses the UTC timezone as the default timezone. The previous code unintentionally marked a not declared timezone as undefined causing the usage of the tickFormatter and niceTickFormatter utility function to consider the timezone as the local browser's timezone.
Before the mentioned PR, we where merging the timezones with the following code:

const timeZones = new Set<string>();
  specs.forEach((spec) => {
    if (spec.timeZone) {
      timeZones.add(spec.timeZone.toLowerCase());
    }
  });
const timeZone = timeZones.size > 1 ? 'utc' : timeZones.values().next().value;

If no timeZone is specified the timeZones Set is empty and the call to timeZones.values().next().value returns undefined that is, on the formatter, considered a local timezone.

The PR instead changed the last code to:

const timeZone = timeZones.size === 1 ? timeZones.values().next().value : 'utc';

where we always have a defined timeZone, that is always UTC when not specified.

Reviewers

I've added but skipped two VRT tests: the function page.emulateTimezone is only available on an higher version of puppeteer, if I update puppeteer in this PR we will get a lot of vrt changes due to the updated chromium version. A subsequent PR will update puppeteer version and unskip the tests

Issues

Solves elastic/kibana#122683 at the root, but as described in the comment the timezone should be specified manually on the props

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)
  • 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 all available themes including dark, light, eui-dark & eui-light

BREAKING CHANGE: the default timezone is now the local browser timezone if not specified.
@markov00 markov00 added :axis Axis related issue :data Data/series/scales related issue :xy Bar/Line/Area chart related labels Jan 12, 2022
@markov00 markov00 marked this pull request as ready for review January 13, 2022 17:38
@markov00 markov00 requested a review from monfera January 13, 2022 17:38
@markov00 markov00 merged commit 1233e69 into elastic:master Jan 13, 2022
@markov00 markov00 deleted the 2022_01_12-fix_default_local_timezone branch January 13, 2022 21:24
@nickofthyme
Copy link
Collaborator

Do we need to backport this into a given kibana version?

nickofthyme pushed a commit that referenced this pull request Jan 14, 2022
# [43.0.0](v42.1.0...v43.0.0) (2022-01-14)

### Bug Fixes

* **heatmap:** labels visibility regression ([#1549](#1549)) ([067189d](067189d))
* **xy:** switch default timezone to `local` ([#1544](#1544)) ([1233e69](1233e69))

### BREAKING CHANGES

* **xy:** The time axis labels of a time-series chart configured without the timeZone prop are now rendering the labels with the local browser timezone instead of UTC.
@monfera
Copy link
Contributor

monfera commented Jan 14, 2022

@nickofthyme the triggering issue #122683 referenced 7.16.2+, worth double checking if it's the exact boundary of the refactoring PR impact, and likely backporting to restore the earlier behavior

@nickofthyme
Copy link
Collaborator

Yeah I don't see a 7.16.4 scheduled so my guess is we just backport into 7.17 for now, agreed?

@monfera
Copy link
Contributor

monfera commented Jan 14, 2022

@nickofthyme good call, and a question is, who we can/should reach out to about a fix release (7.16.4) cc @ghudgins @markov00 would be great to make a decision in proportion to the user impact

@nickofthyme
Copy link
Collaborator

nickofthyme commented Jan 17, 2022

@markov00 after discussing with Stratoula, I don't think it makes sense to backport elastic/kibana#121593 into 7.17 nor 8.0. That said I don't know where to put this fix because if we add it to 7.17 it won't be in 8.0 which would be weird and hence the whole issue we’ve been facing with this late minor. Also 7.17 is on elastic/[email protected] and this change is technically breaking and we already have a 41 version.

Thoughts?

@markov00
Copy link
Member Author

markov00 commented Jan 18, 2022

@nickofthyme So before everything else we have this inconsistency:

  • 7.17 is on 40.2.0 commits
  • 8.0 is on 40.1.1 commits
    I think we should align 8.0 to 40.2.0 so we have also the waffle fix

Then on 40.2.0 we should backport this fix without the breaking change flag. I know that semantically is not correct but we can't do otherwise. This fix needs to land on Kibana 7.17 and 8.0 we can't do otherwise. WDYT?
In the meantime, Infra team have patched 7.16, 7.17, 8.0 and 8.x with elastic/kibana#123139

@nickofthyme
Copy link
Collaborator

Yeah that's fine with me. So release 40.3.0 with the fix and add it to 7.17 and 8.0 (i.e. 8.0.1)?

nickofthyme pushed a commit to nickofthyme/elastic-charts that referenced this pull request Jan 18, 2022
The default timezone used when no timezone prop is provided is now aligned to the browser's local timezone.

BREAKING CHANGE: The time axis labels of a time-series chart configured without the timeZone prop are now rendering the labels with the local browser timezone instead of UTC.
nickofthyme added a commit that referenced this pull request Jan 18, 2022
The default timezone used when no timezone prop is provided is now aligned to the browser's local timezone.

These changes break the time axis labels of a time-series chart configured without the timeZone prop are now rendering the labels with the local browser timezone instead of UTC.

Co-authored-by: Marco Vettorello <[email protected]>
nickofthyme pushed a commit that referenced this pull request Jan 18, 2022
# [40.3.0](v40.2.0...v40.3.0) (2022-01-18)

### Features

* **xy:** switch default timezone to `local` ([#1544](#1544)) ([#1555](#1555)) ([34b848d](34b848d))
@nickofthyme
Copy link
Collaborator

@markov00
Copy link
Member Author

Yes, I think if we can add the fix to 8.0 and 8.0.1 it will be perfect

@nickofthyme
Copy link
Collaborator

Sounds good but to be clear, I'm not adding this to 8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue :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