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

Need update for date-fns 2.0 #7

Closed
sebtiz13 opened this issue Nov 8, 2019 · 12 comments
Closed

Need update for date-fns 2.0 #7

sebtiz13 opened this issue Nov 8, 2019 · 12 comments

Comments

@sebtiz13
Copy link

sebtiz13 commented Nov 8, 2019

Hello

The adapter need an update for version 2.0 of date-fns
Its explain here: https://github.com/date-fns/date-fns/blob/master/docs/unicodeTokens.md

Old:

var FORMATS = {
datetime: 'MMM D, YYYY, h:mm:ss a',
millisecond: 'h:mm:ss.SSS a',
second: 'h:mm:ss a',
minute: 'h:mm a',
hour: 'ha',
day: 'MMM D',
week: 'DD',
month: 'MMM YYYY',
quarter: '[Q]Q - YYYY',
year: 'YYYY'
};

New version

var FORMATS = {
	datetime: 'MMM d, yyyy, h:mm:ss a',
	millisecond: 'h:mm:ss.SSS a',
	second: 'h:mm:ss a',
	minute: 'h:mm a',
	hour: 'ha',
	day: 'MMM d',
	week: 'w',
	month: 'MMM yyyy',
	quarter: '[Q]Q - yyyy',
	year: 'yyyy'
};

May be show the version of date-fns for backward compatibility

@kurkle
Copy link
Member

kurkle commented Nov 8, 2019

Does @next version work for you?

@sebtiz13
Copy link
Author

sebtiz13 commented Nov 8, 2019

@kurkle Yes thanks you its work for me
I'm sorry i didn't see @next version before.
date-fns no longer in @next, it could be communicated in master no ?

@mpskovvang
Copy link

We have discovered a bug in the pre-release (e745f3d) with the week time unit.

The chart won't render with:

...
xAxes: [
  {
    type: 'time',
    time: {
      unit: 'week',
    },
  }
],
...

This works fine with the default moment library.

Let me know if you need an example to reproduce the bug. 🙂

Thanks for a great package. I appreciate the exclusion of the huge moment dependency.

@kurkle
Copy link
Member

kurkle commented Nov 18, 2019

Thanks @mpskovvang!
Pushed new release to @next, can you test it?

@mpskovvang
Copy link

mpskovvang commented Nov 18, 2019

That was fast, thanks!

The chart now renders with the week unit, however, it is still broken with the isoWeekday: true (week starting on Mondays):

weekStartsOn must be between 0 and 6 inclusively

Minor comment: Moment shows weeks as "Nov 10, 2019", but date-fns shows actual week numbers. Perhaps the format can be changed with the adapter options (I haven't tested).

Edited:
The Chart.js documentation mentions these display formats: https://www.chartjs.org/docs/latest/axes/cartesian/time.html#display-formats

The same format as Moment can be achieved with:

...
displayFormats: {
  week: 'PP'
}
...

@kurkle
Copy link
Member

kurkle commented Nov 18, 2019

Interestingly the format was 'PP' in alpha1. Was the discovered bug actually about isoWeekDay?
Can you try with isoWeekDay: 1?

@mpskovvang
Copy link

The bug was discovered with the default value (false) of isoWeekday.

When I found the chart wasn't rendering in the week mode, I switched to moment to verify my implementation. The chart rendered correctly, but I noticed a misplacement between labels and ticks because we use Monday as start of week. That's when I discovered the isoWeekday option in the documentation. moment works fine with and without isoWeekday enabled.

I've tried the adapter both with and without the isoWeekday enabled too, but the alpha2 only works with isoWeekday disabled in my case. The alpha1 doesn't work with week unit at all. 🙂

@kurkle
Copy link
Member

kurkle commented Nov 18, 2019

Did you try alpha2 with isoWeekDay: 1 (number, not true)?

@mpskovvang
Copy link

mpskovvang commented Nov 18, 2019

Sorry, I didn't, because the documentation uses Boolean as the type.

But the number instead of boolean works, thanks! 👍

Would you consider updating the default formats to match the documentation?

Edited
I just realized that this was actually what you changed. Let me try alpha1 again to ensure I wasn't wrong about the bug in the first place.

@kurkle
Copy link
Member

kurkle commented Nov 18, 2019

Pushed alpha3.

@mpskovvang
Copy link

I wasn't able to reproduce the bug in alpha1 - only with isoWeekday: true. I might have lost track of the actual JS build. Apologizes.

However, alpha3 fixes the boolean value with isoWeekday and shows the expected date format.

@kurkle
Copy link
Member

kurkle commented Jan 15, 2020

Version 1.0.0 released

@kurkle kurkle closed this as completed Jan 15, 2020
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

No branches or pull requests

3 participants