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

DatePicker: Fix day's labels for shifting by 1 in specific usr and website timezone combination #67482

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

DatePicker: Fix day's labels for shifting by 1 in specific usr and website timezone combination #67482

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a174b6
add 15 hours to day in datepicker to avoid the DTS impact
MaksVeter Dec 2, 2024
c50efa3
move hours shift closer to the button label print
MaksVeter Dec 2, 2024
ee6229b
Merge branch 'WordPress:trunk' into fix/datepicket-day-labels-uk-dts-bug
MaksVeter Dec 2, 2024
01b104a
Merge branch 'WordPress:trunk' into fix/datepicket-day-labels-uk-dts-bug
MaksVeter Dec 2, 2024
3e1e004
Merge branch 'fix/datepicket-day-labels-uk-dts-bug' of github.com:Mak…
MaksVeter Dec 2, 2024
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
5 changes: 3 additions & 2 deletions packages/components/src/date-time/date/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ function Day( {
// isFocusAllowed is not a dep as there is no point calling focus() on
// an already focused element.
}, [ isFocusable ] );

const shiftedDay = new Date( day );
shiftedDay.setHours( 15 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain a bit more about this technique? Dates have many nuances and would like to understand more about it.

Would similar changes be needed elsewhere or this could resolve this issue for the whole component?

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I'm a bit torn about doing this without any conditions. Shouldn't we be doing it only if we can verify a DST discrepancy is occurring?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that this sets the hours to 15:00:00, assuming that this will keep us in the same day, regardless of which timezone we are in (let me know if that's correct). However, I'm on the right track, this assumption seems flawed: it rests on the assumption that timezones won't spread more than 9 hours in the future or 15 hours in the past. However, AFAIK, timezones can range from UTC−12:00 to UTC+14:00, and that would contradict the initial assumption, and in some cases, this could still be a bug.

I'm also concerned this might create more problems, especially as we reuse time and date components and reuse them in more complex scenarios like DateRange picker (see #60971). The shifted date could then end up being used for time, which, as you can expect, would lead to unexpected results.

return (
<DayButton
ref={ ref }
Expand All @@ -318,7 +319,7 @@ function Day( {
onClick={ onClick }
onKeyDown={ onKeyDown }
>
{ dateI18n( 'j', day, -day.getTimezoneOffset() ) }
{ dateI18n( 'j', shiftedDay, -shiftedDay.getTimezoneOffset() ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

If we go with this approach we'd need to also update the aria-label above.

</DayButton>
);
}
Expand Down
Loading