Skip to content

fix(date-picker): display correct day for first day of month in ar locale #6309

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 24, 2023
Merged

fix(date-picker): display correct day for first day of month in ar locale #6309

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7eeaf1c
fix(date-picker): display correct day for first day of month in ar lo…
anveshmekala Jan 19, 2023
f259799
add screenshot test
anveshmekala Jan 19, 2023
0fa7d0a
repurpose existing test and remove new one
anveshmekala Jan 20, 2023
424a99c
replace hardcoded numbers with const
anveshmekala Jan 20, 2023
31402ec
refactor
anveshmekala Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/components/date-picker-month/date-picker-month.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,16 @@ export class DatePickerMonth {
const date = lastDate.getDate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated nitpick: this method is not following our casing convention and should be getPrevMonthDays (not getPrevMonthdays).

const day = lastDate.getDay();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: For consistency, should this be startDay (similar to the naming in getNextMonthDays)?

const days = [];
if (day - 6 === startOfWeek) {

if (day === (startOfWeek + 6) % 7) {
return days;
}
for (let i = lastDate.getDay() - startOfWeek; i >= 0; i--) {

if (day === startOfWeek) {
return [date];
}

for (let i = (7 + day - startOfWeek) % 7; i >= 0; i--) {
Copy link
Member

@benelan benelan Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mental math is pretty bad so don't hold me to this

I am sure we are on the same boat 😆

Copy link
Contributor Author

@anveshmekala anveshmekala Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

days.push(date - i);
}
return days;
Expand Down
11 changes: 11 additions & 0 deletions src/components/date-picker/date-picker.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,17 @@ export const britishLang_TestOnly = (): string =>
)}
</div>`;

export const arabicLang_TestOnly = (): string =>
html`<div style="width: 400px">
${create(
"calcite-date-picker",
createAttributes({ exceptions: ["lang", "value"] }).concat([
{ name: "lang", value: "ar" },
{ name: "value", value: "2022-08-11" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the value can be added to the existing arabLangNumberingSystem_TestOnly story so we don't need to create a new one?

])
)}
</div>`;

export const chineseLang_TestOnly = (): string =>
html`<div style="width: 400px">
${create(
Expand Down