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

add timezone support #4843

Closed
wants to merge 1 commit into from
Closed

add timezone support #4843

wants to merge 1 commit into from

Conversation

touletan
Copy link
Contributor

Add moment-timezone module to support timezone in time scale
proposed solution to fix this:
Timezone support

Codepen Example

@phbou72
Copy link

phbou72 commented Nov 7, 2017

Would be nice to have this merged!

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

It won't let me comment on the file, but it looks like Chart.js is now a submodule of itself. :|

@touletan
Copy link
Contributor Author

touletan commented Nov 9, 2017

required fix has been applied

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I am ok with the functionality. @benmccann @simonbrunel are you ok with this and bringing in a new dependency?

Copy link
Contributor Author

@touletan touletan left a comment

Choose a reason for hiding this comment

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

.

@benmccann
Copy link
Contributor

Sorry I'm taking awhile to review this. I want to give it really deep thought and do plan to take a look

@benmccann
Copy link
Contributor

benmccann commented Nov 19, 2017

@touletan I really appreciate the hard word you've put into this PR. I've been spending a lot of time digging into this during the past week trying to ensure we take the best path forward. My main concern is that we already get a lot of complaints about how big moment is and it appears that moment-timezone is even larger. It's a really big dependency to be adding and so I don't think it's a good idea to add moment-timezone to Chart.js because of the size.

I reached out to one of the maintainers of moment after coming across their blog while researching possible paths forward and they suggested we might want to consider the new Moment project Luxon: http://moment.github.io/luxon/.

The issue with switching to Luxon is that it is not backwards-compatible. If possible, it would be nice if we didn't leak our usage Moment into our APIs, so that we could make internal changes that users need not be aware of. However, given the situation we are into today I think perhaps this might have to wait until 3.0.0.

Thoughts @etimberg @simonbrunel ?

@etimberg
Copy link
Member

I'm OK with trying out moving to Luxon. Not sure if we want the next version to be 3.0 or not. @simonbrunel what are your thoughts?

@etimberg
Copy link
Member

Closing due to inactivity

@etimberg etimberg closed this Dec 31, 2017
@benmccann
Copy link
Contributor

I've created issue #5186 to consolidate the various pieces of feedback we've received regarding time zone support. Please feel free to subscribe to it for updates

@touletan touletan deleted the timezone branch July 17, 2018 02:09
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

5 participants