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

[chore] manually parse dates #291

Merged
merged 1 commit into from
Feb 7, 2022
Merged

[chore] manually parse dates #291

merged 1 commit into from
Feb 7, 2022

Conversation

chewnoill
Copy link
Contributor

@chewnoill chewnoill commented Feb 3, 2022

This should resolve #281

Javascript's Date.parse was returning inconsistent values depending on the string format, switching to moment avoids this problem. No other changes should be required. Pikaday will use moment by default if it is available.

This does add quite a bit to the js bundle size though.

Alternatively, we could just use a different date format #293

@chewnoill chewnoill added the v4 label Feb 3, 2022
@chewnoill chewnoill requested a review from cpjolicoeur February 3, 2022 15:13
priv/static/torch.js Outdated Show resolved Hide resolved
@chewnoill
Copy link
Contributor Author

Interesting looks like a problem with javascripts Date.parse. Adding moment will fix the problem, because pikaday will use the moment date parser instead

js evaluated (chrome)
new Date("2022/01/11") Tue Jan 11 2022 00:00:00 GMT-0500 (Eastern Standard Time)
new Date("2022-01-11") Mon Jan 10 2022 19:00:00 GMT-0500 (Eastern Standard Time)

@chewnoill chewnoill changed the title [chore] update js bundle [chore] add moment to parse dates Feb 3, 2022
@chewnoill chewnoill force-pushed the wc/update-js-bundle branch from e4e7b14 to d98087d Compare February 3, 2022 18:29
@cpjolicoeur
Copy link
Member

If we are going down the road of adding a dependency, I think I'd rather use something like dayjs or date-fns instead of moment to help reduce size. We can use the parse option of Pikaday to pass in the date parsing function we want to use.

Thoughts?

@chewnoill chewnoill force-pushed the wc/update-js-bundle branch 2 times, most recently from 4f71718 to 4e1dae4 Compare February 7, 2022 19:27
@chewnoill chewnoill changed the title [chore] add moment to parse dates [chore] manually parse dates Feb 7, 2022
@chewnoill chewnoill force-pushed the wc/update-js-bundle branch from 4e1dae4 to 8d8826a Compare February 7, 2022 19:31
@chewnoill chewnoill force-pushed the wc/update-js-bundle branch from 8d8826a to 26b81d0 Compare February 7, 2022 19:32
@chewnoill
Copy link
Contributor Author

I like that waaaay better 👍

@cpjolicoeur
Copy link
Member

@chewnoill I'm going to merge this into v4 but can you also open these same changes against master so that we can push a v3 release with this fix so that we can close out #281?

@cpjolicoeur cpjolicoeur merged commit 06646f5 into v4 Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants