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.

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

🐛(react) fix the DatePicker timezone management #123

Merged
merged 6 commits into from
Aug 2, 2023

Conversation

lebaudantoine
Copy link
Collaborator

@lebaudantoine lebaudantoine commented Jul 12, 2023

Purpose

Fix the component's timezone management.

Proposal

DatePicker and DateRangePicker now support only string as inputs.
They always return an ISO 8601 in UTC as output.

Allowed inputs formats :

  • ISO 8601 date string and time string, with time components and a UTC offset.
  • ISO 8601 date and time string, with time components in UTC time.

Plus, this introduces a new utils function convertDateValueToString. Its responsibilities are :

  • Ensure that any output value is converted back to an ISO 8601 date and time string with in UTC.
  • Harmonize the way we output values.

By introducing this utility function, the proposal aims to ensure consistent and standardized output values across different scenarios.

Finally, a timezone prop has been added to set the timezone of the component. By default, the component's timezone is set to the user locale one. Please read component's documentation, it has been updated with all these recent changes.

@lebaudantoine lebaudantoine added the bug Something isn't working label Jul 12, 2023
@lebaudantoine lebaudantoine self-assigned this Jul 12, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2023

🦋 Changeset detected

Latest commit: 371c2ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@openfun/cunningham-react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lebaudantoine lebaudantoine changed the title Fix/time conversion date picker 🐛(react) Fix the DatePicker's timezone management and offer more flexibility in input and output formats Jul 12, 2023
@lebaudantoine lebaudantoine changed the title 🐛(react) Fix the DatePicker's timezone management and offer more flexibility in input and output formats 🐛(react) Fix the DatePicker's timezone management Jul 12, 2023
@lebaudantoine lebaudantoine changed the title 🐛(react) Fix the DatePicker's timezone management 🐛(react) Fix the DatePicker's timezone management Jul 12, 2023
@lebaudantoine lebaudantoine changed the title 🐛(react) Fix the DatePicker's timezone management 🐛(react) Fix the DatePicker timezone management Jul 12, 2023
@lebaudantoine lebaudantoine changed the title 🐛(react) Fix the DatePicker timezone management 🐛(react) fix the DatePicker timezone management Jul 12, 2023
@lebaudantoine lebaudantoine force-pushed the fix/time-conversion-date-picker branch 2 times, most recently from 1925505 to af572bd Compare July 12, 2023 22:13
Copy link
Contributor

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

A few things, except than that seems good.

Copy link
Member

@NathanVss NathanVss left a comment

Choose a reason for hiding this comment

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

I'm wondering what motivated this change ?

IMO we could think of a way to streamline more simply the date format management:

  • The value props could be Date | undefined
  • The onChange callback attrbitute could be Date | undefined

From my perspective we should avoid manipulating dates without the TZ, it lets the door opened to lots of TZ related bugs. A date without the TZ has no meaning, so in my opinion it could be considered as a bad practice.

This could led to potential problems when dealing with world-wide users, for instance 01/01/2023 00:00:00 has a different meaning in each 1000km separated territories accros the earth, so there is a door opened to subjective interpretation as it lacks informations. While 01/01/2023 00:00:00 Z as the same meaning anywhere on earth ( Z = UTC ), no subjective interpretation is possible and we will never face a situation where the calendar shows the same date in the US and in Mumbai .. which is a non sense: it should show different dates relative to their TZ.

A calendar with the value 01/01/2023 00:00:00 Z should display:
31/12/2022 in the US
01/01/2023 in Europe
01/01/2023 in Asia

I faced lots of this issues when working for a world-wide platform with users from San Fransisco to Shanghai and that's one of my main return on experience. 😅

Just to avoid any misconception: I'm just saying that this could led to mis-usages which we should maybe hard-prevent.

( WDYT @jbpenrath @AntoLC ) ?

@lebaudantoine
Copy link
Collaborator Author

lebaudantoine commented Jul 13, 2023

@NathanVss

I acknowledge the potential shortcomings in component’s timezone management. I understand and share your perspective. Here are few responses:


IMO we could think of a way to streamline more simply the date format management […]

I agree that having a single type of input, whether it's a string or a Date, would be an improvement.

I like your suggestion. The JavaScript Date object doesn't inherently store timezone information. It represents a moment in time without built-in capabilities for managing timezones. I'll take some time to consider your suggestion.

With react-aria, the ZonedDateTime type doesn't have a built-in constructor from a JS Date object. Therefore, internal conversion to a string is still required. However, ZonedDateTime does offer a toDate() method, which can be used to obtain a JavaScript Date object.


I'm wondering what motivated this change ?

The current date picker lacks the granularity to select a specific time, making it feel arbitrary to restrict the selection to only a date with a fixed and default time. This approach of forcing a time at midnight can result in different dates across timezones, which appears odd for a date picker designed primarily for selecting a day rather than a specific time.

I intended to leverage the various DateValue types provided by react-aria, including CalendarDate, CalendarDateTime, and ZonedDateTime.


This could led to potential problems when dealing with world-wide users […]

Developers can still pass an ISO 8601 string with a UTC offset to ensure proper conversion of the date to the user's local timezone. However, one area that could be improved is the situation where the date is cleared, resulting in the loss of time and timezone information. However, agreed, relying solely on the developer for this responsibility may not offer the most optimal developer experience.


I'm just saying that this could led to mis-usages which we should maybe hard-prevent.

Acknowledging that my proposal may introduce additional complexity, it is important to note that it also provides a significant advantage in terms of freedom and flexibility. Considering this, would you like me to reconsider the component design, incorporating your suggested default behavior? Your suggestion might simplify developer experience while using the component.

@lebaudantoine
Copy link
Collaborator Author

(This PR is the result of the discussion we had before our mutual holidays @jbpenrath about setting component selected date time to noon instead of midnight.)

@lebaudantoine lebaudantoine force-pushed the fix/time-conversion-date-picker branch from e487545 to 63d3fe4 Compare July 18, 2023 23:56
@lebaudantoine lebaudantoine force-pushed the fix/time-conversion-date-picker branch 6 times, most recently from fff6ca6 to 739b8b9 Compare July 25, 2023 23:11
Copy link
Member

@jbpenrath jbpenrath left a comment

Choose a reason for hiding this comment

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

Nice job ! 👏

@lebaudantoine lebaudantoine force-pushed the fix/time-conversion-date-picker branch 4 times, most recently from c030ffa to 0e53bcb Compare August 2, 2023 16:31
@lebaudantoine lebaudantoine force-pushed the fix/time-conversion-date-picker branch from 0e53bcb to 6eaca12 Compare August 2, 2023 16:34
By introducing this utility function, we ensure that any output value
is converted back to an ISO 8601 date and time string in UTC timezone.
Overall, it harmonize and factorize the way we output
values from date picker components.
Enforces a standardized approach for end consumers using the
component's API. Consumers are now required to provide a date as an
ISO string in either UTC or with a UTC offset. The support for native
Date objects has been removed to ensure consistent and reliable
behavior across all implementations. By adopting this uniform input
format, we can simplify usage and avoid potential inconsistencies in
date handling.
By default, component's timezone is the user locale timezone.
Component now offers a way to set its timezone to any supported
Intl timezone format. Please note that output values from the
component will always be converted to a UTC timezone.
Add some docstring documentation to the newly updated utils
functions, to help understand what are their role and params.
User have a clear view of inputs and outputs timezone.
Describe the current behavior of date picker components,
especially with the recent changes in component's API.
In order to use vitest dependency, make the utils test file
use a tsx extension.
@lebaudantoine lebaudantoine force-pushed the fix/time-conversion-date-picker branch from 6eaca12 to 371c2ad Compare August 2, 2023 16:54
@lebaudantoine lebaudantoine merged commit 94ae861 into main Aug 2, 2023
@lebaudantoine lebaudantoine deleted the fix/time-conversion-date-picker branch August 2, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants