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.

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

fix: align days and week days on calendar #31641

Merged
merged 11 commits into from
Dec 28, 2023
Merged

fix: align days and week days on calendar #31641

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
72fc564
extract hardcoded weekStartsOn into a method inside DateUtils
barros001 Nov 20, 2023
3dd8f85
fixed usage of DateUtils lib
barros001 Nov 20, 2023
a13abd6
moved week starts on into a constant
barros001 Nov 21, 2023
5b6b2ff
updated generateMonthMatrix test
barros001 Nov 21, 2023
aa015de
updated how empty days are added at the beginning of the month
barros001 Nov 21, 2023
d4a2c58
Merge branch 'main' into calendar-fix
barros001 Dec 1, 2023
a355972
Merge branch 'main' into calendar-fix
barros001 Dec 5, 2023
f9d8b86
Merge branch 'main' into calendar-fix
barros001 Dec 12, 2023
82bc4d7
Merge branch 'main' into calendar-fix
barros001 Dec 19, 2023
026ee86
Merge branch 'main' into calendar-fix
barros001 Dec 26, 2023
5fb6e98
add a few more tests
barros001 Dec 26, 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
1 change: 1 addition & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ const CONST = {
MAX_PENDING_TIME_MS: 10 * 1000,
MAX_REQUEST_RETRIES: 10,
},
WEEK_STARTS_ON: 1, // Monday
MonilBhavsar marked this conversation as resolved.
Show resolved Hide resolved
DEFAULT_TIME_ZONE: {automatic: true, selected: 'America/Los_Angeles'},
DEFAULT_ACCOUNT_DATA: {errors: null, success: '', isLoading: false},
DEFAULT_CLOSE_ACCOUNT_DATA: {errors: null, success: '', isLoading: false},
Expand Down
17 changes: 11 additions & 6 deletions src/components/DatePicker/CalendarPicker/generateMonthMatrix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {addDays, format, getDay, getDaysInMonth, startOfMonth} from 'date-fns';
import DateUtils from '@libs/DateUtils';

/**
* Generates a matrix representation of a month's calendar given the year and month.
Expand All @@ -24,6 +25,9 @@ export default function generateMonthMatrix(year, month) {
throw new Error('Month cannot be greater than 11');
}

// Get the week day for the end of week
const weekEndsOn = DateUtils.getWeekEndsOn();

// Get the number of days in the month and the first day of the month
const firstDayOfMonth = startOfMonth(new Date(year, month, 1));
const daysInMonth = getDaysInMonth(firstDayOfMonth);
Expand All @@ -32,18 +36,13 @@ export default function generateMonthMatrix(year, month) {
const matrix = [];
let currentWeek = [];

// Add null values for days before the first day of the month
for (let i = 0; i < getDay(firstDayOfMonth); i++) {
currentWeek.push(null);
}

// Add calendar days to the matrix
for (let i = 1; i <= daysInMonth; i++) {
const currentDate = addDays(firstDayOfMonth, i - 1);
currentWeek.push(Number(format(currentDate, 'd')));

// Start a new row when the current week is full
if (getDay(currentDate) === 6) {
if (getDay(currentDate) === weekEndsOn) {
matrix.push(currentWeek);
currentWeek = [];
}
Expand All @@ -56,5 +55,11 @@ export default function generateMonthMatrix(year, month) {
}
matrix.push(currentWeek);
}

// Add null values for days before the first day of the month
for (let i = matrix[0].length; i < 7; i++) {
matrix[0].unshift(null);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change? push vs unshift

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unshift will add the empty days at the beginning of the calendar. The way it was done before (see snippet below), it would FIRST add the empty spaces (push) but now we add all the days first and only then we add the empty spaces at the beginning (unshift)

    for (let i = 0; i < getDay(firstDayOfMonth); i++) {
        currentWeek.push(null);
    }

this no longer works because now our week starts on 1 and ends on 0 (and it could change). This breaks as the start is higher than the end (1,2,3,4,5,6,0).

hope this makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am on mobile so I missed on thing. The snippet I pasted above (old code) would go from 0 (start of the week assumed earlier - which is now wrong) until the first week day of the month (which before would always be 0 or higher). Now, because week starts on 1 (Sunday) the first week day of a month could be Sunday (0) and the loop would break and cause infinite loop. My change (unshift) accounts for any start of the week and any first week day of the month.

return matrix;
}
29 changes: 25 additions & 4 deletions src/libs/DateUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import * as Localize from './Localize';
type CustomStatusTypes = (typeof CONST.CUSTOM_STATUS_TYPES)[keyof typeof CONST.CUSTOM_STATUS_TYPES];
type TimePeriod = 'AM' | 'PM';
type Locale = ValueOf<typeof CONST.LOCALES>;
type WeekDay = 0 | 1 | 2 | 3 | 4 | 5 | 6;

let currentUserAccountID: number | undefined;
Onyx.connect({
Expand Down Expand Up @@ -68,6 +69,22 @@ Onyx.connect({
},
});

/**
* Get the day of the week that the week starts on
*/
function getWeekStartsOn(): WeekDay {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also be renamed to getWeekStartDay and similar for end function below

Copy link
Contributor Author

@barros001 barros001 Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@MonilBhavsar the idea was to follow the same naming convention used date-fns which is the underlying library we use:

https://github.com/date-fns/date-fns/blob/a39b8058f777517e8040193ba597dff18765951a/src/types.ts#L192-L195

This allows us to do things like this:

    const weekStartsOn = getWeekStartsOn();
    const startOfCurrentWeek = startOfWeek(new Date(), {weekStartsOn});
    const endOfCurrentWeek = endOfWeek(new Date(), {weekStartsOn});

instead of:

    const weekStartDay = getWeekStartDay();
    const startOfCurrentWeek = startOfWeek(new Date(), {weekStartsOn: weekStartDay});
    const endOfCurrentWeek = endOfWeek(new Date(), {weekStartsOn: weekStartDay});

IMO more readable and consistent to keep it as is (WEEK_STARTS_ON, getWeekStartsOn and getWeekEndsOn).

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense 👍
let's keep it that way. thanks!

return CONST.WEEK_STARTS_ON;
}

/**
* Get the day of the week that the week ends on
*/
function getWeekEndsOn(): WeekDay {
const weekStartsOn = getWeekStartsOn();

return weekStartsOn === 0 ? 6 : ((weekStartsOn - 1) as WeekDay);
Comment on lines +84 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

start day is always constant, so this is not required now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it's not required, but the idea was to allow for flexibility. If we ever decide to switch to Sunday (which is the standard in the US, where most of our customers are), we won't have to remember to change it here as well (or have two constants). Also, there seem to be an appetite to make the start of the week dynamic at some point based on locale, so this function will be ready for it when/if that day comes.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}

/**
* Gets the locale string and setting default locale for date-fns
*/
Expand Down Expand Up @@ -153,9 +170,10 @@ function datetimeToCalendarTime(locale: Locale, datetime: string, includeTimeZon
let tomorrowAt = Localize.translate(locale, 'common.tomorrowAt');
let yesterdayAt = Localize.translate(locale, 'common.yesterdayAt');
const at = Localize.translate(locale, 'common.conjunctionAt');
const weekStartsOn = getWeekStartsOn();

const startOfCurrentWeek = startOfWeek(new Date(), {weekStartsOn: 1}); // Assuming Monday is the start of the week
const endOfCurrentWeek = endOfWeek(new Date(), {weekStartsOn: 1}); // Assuming Monday is the start of the week
const startOfCurrentWeek = startOfWeek(new Date(), {weekStartsOn});
const endOfCurrentWeek = endOfWeek(new Date(), {weekStartsOn});

if (isLowercase) {
todayAt = todayAt.toLowerCase();
Expand Down Expand Up @@ -292,8 +310,9 @@ function getDaysOfWeek(preferredLocale: Locale): string[] {
if (preferredLocale) {
setLocale(preferredLocale);
}
const startOfCurrentWeek = startOfWeek(new Date(), {weekStartsOn: 1}); // Assuming Monday is the start of the week
const endOfCurrentWeek = endOfWeek(new Date(), {weekStartsOn: 1}); // Assuming Monday is the start of the week
const weekStartsOn = getWeekStartsOn();
const startOfCurrentWeek = startOfWeek(new Date(), {weekStartsOn});
const endOfCurrentWeek = endOfWeek(new Date(), {weekStartsOn});
const daysOfWeek = eachDayOfInterval({start: startOfCurrentWeek, end: endOfCurrentWeek});

// eslint-disable-next-line rulesdir/prefer-underscore-method
Expand Down Expand Up @@ -707,6 +726,8 @@ const DateUtils = {
getMonthNames,
getDaysOfWeek,
formatWithUTCTimeZone,
getWeekStartsOn,
getWeekEndsOn,
isTimeAtLeastOneMinuteInFuture,
};

Expand Down
62 changes: 31 additions & 31 deletions tests/unit/generateMonthMatrixTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,67 +3,67 @@ import generateMonthMatrix from '../../src/components/DatePicker/CalendarPicker/
describe('generateMonthMatrix', () => {
it('returns the correct matrix for January 2022', () => {
const expected = [
[null, null, null, null, null, null, 1],
[2, 3, 4, 5, 6, 7, 8],
[9, 10, 11, 12, 13, 14, 15],
[16, 17, 18, 19, 20, 21, 22],
[23, 24, 25, 26, 27, 28, 29],
[30, 31, null, null, null, null, null],
[null, null, null, null, null, 1, 2],
[3, 4, 5, 6, 7, 8, 9],
[10, 11, 12, 13, 14, 15, 16],
[17, 18, 19, 20, 21, 22, 23],
[24, 25, 26, 27, 28, 29, 30],
[31, null, null, null, null, null, null],
];
expect(generateMonthMatrix(2022, 0)).toEqual(expected);
});

it('returns the correct matrix for February 2022', () => {
const expected = [
[null, null, 1, 2, 3, 4, 5],
[6, 7, 8, 9, 10, 11, 12],
[13, 14, 15, 16, 17, 18, 19],
[20, 21, 22, 23, 24, 25, 26],
[27, 28, null, null, null, null, null],
[null, 1, 2, 3, 4, 5, 6],
[7, 8, 9, 10, 11, 12, 13],
[14, 15, 16, 17, 18, 19, 20],
[21, 22, 23, 24, 25, 26, 27],
[28, null, null, null, null, null, null],
];
expect(generateMonthMatrix(2022, 1)).toEqual(expected);
});

it('returns the correct matrix for leap year February 2020', () => {
const expected = [
[null, null, null, null, null, null, 1],
[2, 3, 4, 5, 6, 7, 8],
[9, 10, 11, 12, 13, 14, 15],
[16, 17, 18, 19, 20, 21, 22],
[23, 24, 25, 26, 27, 28, 29],
[null, null, null, null, null, 1, 2],
[3, 4, 5, 6, 7, 8, 9],
[10, 11, 12, 13, 14, 15, 16],
[17, 18, 19, 20, 21, 22, 23],
[24, 25, 26, 27, 28, 29, null],
];
expect(generateMonthMatrix(2020, 1)).toEqual(expected);
});

it('returns the correct matrix for March 2022', () => {
const expected = [
[null, null, 1, 2, 3, 4, 5],
[6, 7, 8, 9, 10, 11, 12],
[13, 14, 15, 16, 17, 18, 19],
[20, 21, 22, 23, 24, 25, 26],
[27, 28, 29, 30, 31, null, null],
[null, 1, 2, 3, 4, 5, 6],
[7, 8, 9, 10, 11, 12, 13],
[14, 15, 16, 17, 18, 19, 20],
[21, 22, 23, 24, 25, 26, 27],
[28, 29, 30, 31, null, null, null],
];
expect(generateMonthMatrix(2022, 2)).toEqual(expected);
});

it('returns the correct matrix for April 2022', () => {
const expected = [
[null, null, null, null, null, 1, 2],
[3, 4, 5, 6, 7, 8, 9],
[10, 11, 12, 13, 14, 15, 16],
[17, 18, 19, 20, 21, 22, 23],
[24, 25, 26, 27, 28, 29, 30],
[null, null, null, null, 1, 2, 3],
[4, 5, 6, 7, 8, 9, 10],
[11, 12, 13, 14, 15, 16, 17],
[18, 19, 20, 21, 22, 23, 24],
[25, 26, 27, 28, 29, 30, null],
];
expect(generateMonthMatrix(2022, 3)).toEqual(expected);
});

it('returns the correct matrix for December 2022', () => {
const expected = [
[null, null, null, null, 1, 2, 3],
[4, 5, 6, 7, 8, 9, 10],
[11, 12, 13, 14, 15, 16, 17],
[18, 19, 20, 21, 22, 23, 24],
[25, 26, 27, 28, 29, 30, 31],
[null, null, null, 1, 2, 3, 4],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a matrix for some month of year 2025, may be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MonilBhavsar added a few more tests including a month that starts on Monday, one that starts on Sunday, February (less days) and one more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

[5, 6, 7, 8, 9, 10, 11],
[12, 13, 14, 15, 16, 17, 18],
[19, 20, 21, 22, 23, 24, 25],
[26, 27, 28, 29, 30, 31, null],
];
expect(generateMonthMatrix(2022, 11)).toEqual(expected);
});
Expand Down
Loading