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: Calendar fix for DST #12172

Merged
merged 1 commit into from Apr 20, 2020
Merged

Fix: Calendar fix for DST #12172

merged 1 commit into from Apr 20, 2020

Conversation

MYKEU
Copy link
Contributor

@MYKEU MYKEU commented Feb 19, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fixes calendar chart layout when user is in a timezone with DST

Fixed issues

#12143
#10430

Details

Before: What was the problem?

When user is in a timezone with DST, the last several months of the year on the calendar chart begin to overlap eachother.

74432963-69a89a80-4e57-11ea-82b1-8320fea36821

After: How is it fixed in this PR?

The fix is based on the solution from #10430

Screenshot 2020-02-19 at 17 04 17

There is currently another PR attempting to fix this problem here: #12081 - however, from testing, it did not appear to correct the problem.

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Feb 19, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

@MYKEU MYKEU requested a review from Ovilia February 21, 2020 12:55
@Ovilia Ovilia added this to the 4.8.0 milestone Feb 28, 2020
@damiangreen
Copy link

This fix works great for us, would be nice to get this merged

@Ovilia
Copy link
Contributor

Ovilia commented Mar 27, 2020

@damiangreen This PR is schedule for 4.8.0 so it will be reviewed soon. Thanks!

@damiangreen
Copy link

Thanks @Ovilia , we're really looking forward to it

@100pah 100pah merged commit 630551b into apache:master Apr 20, 2020
@echarts-bot
Copy link

echarts-bot bot commented Apr 20, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@damiangreen
Copy link

Great! When will it hit npm?

@Ovilia
Copy link
Contributor

Ovilia commented Apr 21, 2020

@damiangreen We need a few more weeks to go through Apache release progress, which usually takes about 2-4 weeks.

@MYKEU
Copy link
Contributor Author

MYKEU commented Apr 23, 2020

@Ovilia I have checked the master branch recently and noticed that my changes have been reverted - do you mind confirming what happened please? Possible merge conflict issue?

https://github.com/apache/incubator-echarts/commits/master/src/coord/calendar/Calendar.js

@100pah
Copy link
Member

100pah commented Apr 24, 2020

@mikeyshing88 Thanks for your carefully check. Sorry I should have made some comment.
The PR is merged. But then I made little change based on the code located, because it seams not entirely perfect considering the context surrounding the code.
The code snippet behind the modified code was added before to the resolve another DST issue. But it did not do it correct in some cases we currently found. I modify it to cover this cases. I am not totally sure it is a best way to resolve DST issue in that way. But logically it seams correct to cover all of the cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants