Add week numbers #50
Add week numbers #50
Conversation
|
Hi @viljamis! Could you please take a look? Currently it still lacks end-to-end test but I was wandering what else needs to be done for this feature to get merged. |
| @@ -77,6 +85,7 @@ export const DatePickerMonth: FunctionalComponent<DatePickerMonthProps> = ({ | |||
| <tbody> | |||
| {chunk(days, 7).map(week => ( | |||
| <tr class="duet-date__row"> | |||
| {weekNumbers && <td class="duet-date__week">{weekOfYear(week[0])}</td>} | |||
There was a problem hiding this comment.
I think you need scope="row" role="rowheader" here to properly indicate this is a header for the row.
| const start = startOfWeek(new Date(startYear, 0, 4)) | ||
| const end = startOfWeek(date) | ||
| const diff = end.getTime() - start.getTime() | ||
| return Math.round(diff / MILLISECONDS_IN_WEEK) + 1 |
There was a problem hiding this comment.
I'm curious about the implementation of this. It's very hard to know if this gives correct results for every possibility. Is this lifted from somewhere (which is fine but a link in a comment would be useful), or have you written it yourself? Are you able to give a walk through of what's going on here?
I'm curious about the use of 4th January. And also whether this would be affected by (and therefore should take as an argument) the first day of the week? I assume so
There was a problem hiding this comment.
Here are the rules of how to determine week numbers https://en.wikipedia.org/wiki/Week#Week_numbering
This version returns ISO week number and is healivy inspired by momentjs and date-fns packages. Both of them use 4th january to find first week.
In my use-case iso week number made most sense as it matches values that PostgreSQL is returning(SELECT EXTRACT('isoyear' FROM '2021-01-03'::date), EXTRACT('week' FROM '2021-01-03'::date)) but there are other calendar apps with different logic. For example Apple Calendar 2020-12-28 is recognised as 1st week and not 53.
date-fns package getWeek function has two options - weekStartsOn and firstWeekContainsDate so solve this.
How do you feel about adding momentjs or date-fns as a dependency? Or should we just reimplement the function here as well?
There was a problem hiding this comment.
@WickyNilliams Have you had time to decide how this could be continued?
There was a problem hiding this comment.
Hey sorry about the delay @krists. Have a lot going on at the moment, so it's hard to find time to dedicate to this.
Oh that wikipedia article will be very useful. I started researching how to calculate this before and wasn't finding any good information.
I don't want to add date-fns or moment as a dependency, since we were very focused on making this picker as lightweight as possible.
I'll take a closer look at the logic here later in the week
There was a problem hiding this comment.
@WickyNilliams understandable. jquery-ui library has similar functionality as well. It provides a option to override default implemenation.
This pull requests adds option to display week numbers. This feature was requested in #24