fix(date-picker): display correct day for first day of month in ar locale
#6309
Conversation
anveshmekala
added
bug
| "calcite-date-picker", | ||
| createAttributes({ exceptions: ["lang", "value"] }).concat([ | ||
| { name: "lang", value: "ar" }, | ||
| { name: "value", value: "2022-08-11" } |
There was a problem hiding this comment.
Maybe the value can be added to the existing arabLangNumberingSystem_TestOnly story so we don't need to create a new one?
| return [date]; | ||
| } | ||
|
|
||
| for (let i = (7 + day - startOfWeek) % 7; i >= 0; i--) { |
There was a problem hiding this comment.
My mental math is pretty bad so don't hold me to this, but it seems like these changes could effect more than just a single locale. I was expecting some kind of change in the nls data - don't we have a weekStart property?
The linked PR seems unrelated so I'm not sure what the exact issue is, can you link the correct one?
Nitpick:
In general I'm not a big fan of "magic numbers". It could also be nice to move this logic to a functional util in date.ts and add some spec tests to make sure it's always doing what we expect.
There was a problem hiding this comment.
My mental math is pretty bad so don't hold me to this
I am sure we are on the same boat 😆
There was a problem hiding this comment.
The linked PR is not the root cause, the earlier logic of determining which dates from previous months to display is what causing the issue when the weekStart is set to 6 in the nls data.
Irrespective of the weekStart inside the nls data one algorithm should work for all . One of differences is that CLDR data has weekStart from 1 to 7 where as getDay( ) method return indexes from 0 to 6 ( 0 being Sunday).
Other issue ( the root cause) is that we can't directly use difference in indexes to determine which dates from previous month should be rendered in current because if the weekStart is set to Saturday (6) and the last day of previous month is Monday(1) , the difference is 5 which means we need to display last 5days of previous month. But in reality, we just need to need to display Saturday, Sunday, Monday from previous month dates. To remove the circular number system ( there might be right term for this , but this is how i imagined in my mind lol) , i added 7 so that any date can be compared like normal numbers.
I tried multiple combinations and looked at code piece for getNextMonthDays which is doing similar thing but adding 6 so that we get to fill the rest of the last week in current month with next month dates.
I agree complex logic like these are hard to review and maintain but it is unavoidable in this case .
There was a problem hiding this comment.
Could you at least create a const for 7 to eliminate some of the magic numberness? For example const DAYS_PER_WEEK = 7
Creating one for 6 may be nice too to explain why that number is being used
anveshmekala
added
pr ready for visual snapshots
anveshmekala
added
pr ready for visual snapshots
| @@ -13,6 +13,8 @@ import { dateFromRange, HoverRange, inRange, sameDate } from "../../utils/date"; | |||
| import { DateLocaleData } from "../date-picker/utils"; | |||
| import { Scale } from "../interfaces"; | |||
|
|
|||
| const DAYS_PER_WEEK = 7; | |||
| const DAYS_MAXIMUM_INDEX = 6; | |||
| @Component({ | |||
There was a problem hiding this comment.
Nitpick: can you add a newline here to visually separate const declaration from the component?
| @@ -277,10 +279,16 @@ export class DatePickerMonth { | |||
| const date = lastDate.getDate(); | |||
There was a problem hiding this comment.
Unrelated nitpick: this method is not following our casing convention and should be getPrevMonthDays (not getPrevMonthdays).
| @@ -277,10 +279,16 @@ export class DatePickerMonth { | |||
| const date = lastDate.getDate(); | |||
| const day = lastDate.getDay(); | |||
There was a problem hiding this comment.
Nitpick: For consistency, should this be startDay (similar to the naming in getNextMonthDays)?
anveshmekala
added
pr ready for visual snapshots
Related Issue: #6182
Summary
This PR will display correct day for first day the month in
arlocale.Note: No changes expected for other locales. Current screenshots will cover locales whose weekStart index is either 1,6 or 7. There aren't any other locales whose weekStart index falls outside of 1,6 or 7.