-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Week scale - enhance and add to auto-scaling #3549
Conversation
@@ -73,7 +73,7 @@ TimeStep.FORMAT = { | |||
hour: 'HH:mm', | |||
weekday: 'ddd D', | |||
day: 'D', | |||
week: 'w', | |||
week: 'D', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why to choose the "Day of Month" as default instead of "Locale week of year" when local-aware moment.js is defined as a requirement. Besides, it seems to be more safe to choose the local-aware moment.js week, you never know what happens within these locales.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for that is to be consistent with the other formats, so when your zooming it doesn't jump from number of the week in the year to day of the month.
This can then be overridden to fit specific cases using format: { minorLabels: {week: 'w'} }
as shown in this example: weekStyling
lib/timeline/TimeStep.js
Outdated
} | ||
} | ||
break; | ||
case 'week': this.current.add(this.step, 'week'); break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I absolutely disagree with this change, which causes the first day of the month to completely disappear. In fact, I believe that showing the start of the month is mandatory, it is at least for me and so far I haven't seen anybody complaining. Week starts do not necessarily align with the start of the month. With this change, there is wrong information displayed, e.g. giving the user the impression that the month starts right with the start of the week, as is visible in your initial comment for this PR #3549, e.g. in September 2016, (ISO) week 37 started on the 12th of September. Also it is not consistent, because July looks like it was 5 weeks long. That's why I went through all the hassle with this admittedly ugly piece of code. But please leave it as it is, and maybe apply your own patch or make it configurable if you don't like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that this changes a bit your initial implementation, but it's more generic and consistent with the rest of the code, i.e. creates a regular scale that iterates only weeks (as we are creating a week scale), without exceptions.
The rest is just a matter of label formatting, if the majorLabel seems a bit misleading it can be changed for D MMM YYYY instead. But again, this can be modified using format: { majorLabels: {week: 'D MMM YYYY'} }
It's not a matter of disliking the current solution, it's instead an attempt to make it more consistent with the rest of the code, usable for other cases and customizable.
You're right about those issues. Months as majorLabels and week numbers as minorLabels don't work really well together, perhaps because 'week numbers' are relative to the year...
Thanks for reviewing the code. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point and agree that this approach is more consistent. But I'd like to suggest to at least change the major label default to D MMM YYYY if you think this change is absolutely necessary. Otherwise you get the impression of a wrong start of the month. For me, this change makes the week scale more or less useless because I really need and want to see the beginning of the month. What imho is really needed is a more flexible/independent approach to draw the major and minor labels and lines as requested in #3323. Please reconsider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree with @marcortw . The weeks don't always align with the month so the current behavior is much preferred to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have to agreed with you guys. It's rather confusing. I'll revert this change and keep the current behavior. ;)
Can we still add the week scale to the auto-scale? It could replace the 5 days scale (or keep both?) and by default its minorLabels would display the "day of the month". What do you guys think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that, thanks! I initially had the weeks in the auto-scale but then left it out because I thought that people might be confused by the week number, it's probably a bit of information that is not used by many people.
@knokit Can you please fix the build? Travis reports the following:
|
@wimrijnders, not sure how can I fix it. I haven't change anything connected with the tests and they all passed when I ran them locally. Any suggestions? |
OK understood. This has happened before. The thing to do here is to retrigger the travis test by submitting a new commit. I suggest you merge the PR with the latest |
6339364
to
fb4965f
Compare
Yay! 👍 |
@wimrijnders, thanks! 👍 |
Thank you for getting this done! |
Travis test failing not your fault. I had this zany idea to add a stress test to the unit tests, and sometimes it exceeds the time out. Remedy is same as before; otherwise I'll adjust the timeout in question. Edit: You might try merging with |
* next() always adds 1 week to current date * show 1st week in the month as majorLabel
This reverts commit 52df3c3.
f23f636
to
35ab69a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for the PR!
* Move majorLables to 1st week in the month, for 'week' scale * next() always adds 1 week to current date * show 1st week in the month as majorLabel * Add 'week' to auto-scale * Update week scale example * Revert "Move majorLables to 1st week in the month, for 'week' scale" This reverts commit 52df3c3. * Correct value of week's minimumStep
Changes:
Makes the week scale more consistent and removes clutter:next() always adds 1 week to current datemove majorLabels to the 1st week in the monthScreenshots:
![week_scale_orig](https://user-images.githubusercontent.com/3659829/31429865-72b981c8-ae6f-11e7-9ac0-67c7f5ad2c76.png)
Before this PR: (keep this behavior)
After this PR:(reverted)Live examples: