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

Not able to set default time zone for DatePicker and TimeInput #3432

Closed
ArnaudValensi opened this issue Jan 27, 2023 · 19 comments
Closed

Not able to set default time zone for DatePicker and TimeInput #3432

ArnaudValensi opened this issue Jan 27, 2023 · 19 comments

Comments

@ArnaudValensi
Copy link

ArnaudValensi commented Jan 27, 2023

What package has an issue

@mantine/dates

Describe the bug

All the dates displayed by the DatePicker and TimeInput are in the local timezone. There is no way to display it using a custom timezone, like UTC for example.

What version of @mantine/hooks page do you have in package.json?

5.10.2

Some comments

Date object always store absolute dates inside, this is when we use the function toString or the functions to get any of the component that it does the shift based on the local timezone. TimeInput just uses those functions to display the dates, so it is not possible to display a date as UTC for example without creating fake dates.

Right now the only solution I have is to do this:

<TimeInput
    value={subtractUtcOffset(date)}
    onChange={(value: Date) => onTimeChange(addUtcOffset(value))}
/>

function subtractUtcOffset(date: Date) {
    if (!date) {
        return date;
    }

    let result = dayjs(date);

    const utcOffsetInMinutes = dayjs().utcOffset();
    result = result.subtract(utcOffsetInMinutes, 'minutes');

    return result.toDate();
}

function addUtcOffset(date: Date) {
    if (!date) {
        return date;
    }

    let result = dayjs(date);

    const utcOffsetInMinutes = dayjs().utcOffset();
    result = result.add(utcOffsetInMinutes, 'minutes');

    return result.toDate();
}

Note that the dates created with the shift functions are date that are not representing the real time we manipulate. This is why having a way to select the timezone in the DatePicker and TimeInput components would be better.

Also, this code is bugged, it uses the current UTC offset, but the offset changes depending on the daylight saving in some countries. So if we are in winter and I want to select an UTC date in summer, this solution will give me an incorrect date.

@benlongo
Copy link
Contributor

benlongo commented Mar 15, 2023

This issue is still present with the @mantine/dates 6.0 overhaul.

If mantine is to use Date objects (which represent an absolute point in time), as opposed to some timezone-oblivious object, then I think it must be timezone aware to result in correct behavior when used with timezones that are different than the browsers timezone. As pointed out here, the UTC offset workaround is insufficient.

It could be decided that dealing with timezones that are different than the browsers timezone is out of scope, but this seems like a pretty significant limitation for apps that deal with objects across timezones.

I propose adding an optional tz (or timezone) prop to anything that deals with Date objects and also extending DatesProvider to hold a default timezone.

@rtivital any objections to a PR?

@rtivital
Copy link
Member

You are welcome to submit a PR with timezone on DatesProvider

@benlongo
Copy link
Contributor

benlongo commented Mar 16, 2023

Would you also be open to adding a prop to all relevant components? I suppose that could be done as a second stage

@rtivital
Copy link
Member

I think timezone on DatesProvider is enough

@wuifdesign
Copy link
Contributor

Hi, is there any update to this? or will this be addressed with the v7 rework?

@rtivital
Copy link
Member

rtivital commented Sep 1, 2023

There is no rework of the dates package in v7. If you are interested in contributing this feature, you can submit a PR in mantine-v7 repo

@wuifdesign
Copy link
Contributor

@rtivital what is with #3823? wouldn't that work for v7?

@rtivital
Copy link
Member

rtivital commented Sep 1, 2023

It is incomplete and abandoned

@wuifdesign
Copy link
Contributor

i'll take a look at it, currently dayjs and timezones is a little bugged (iamkun/dayjs#2398 and many more). do you have any idea to implement timezones easily? or will it just be a bigger rework? as i think this has to be implementet in may of the components.?

@benlongo
Copy link
Contributor

benlongo commented Sep 3, 2023

Unfortunately the temporal API hasn't landed yet, but it would be a good API for this

@wuifdesign
Copy link
Contributor

wuifdesign commented Sep 4, 2023

would be switching form exposing Date objects to dayjs objects? this will be a breaking change.
i can't think of a way of handling timezones with the default javascript Date Objects. do you have any idea about it?

it may be possible to expose date objects for the components, but as you also export helper funtions this will be very hard for them.

@benlongo
Copy link
Contributor

benlongo commented Sep 5, 2023

I was referring to the (currently) stage 3 TC39 proposal (https://tc39.es/proposal-temporal/docs/) which has a much better API than dayjs and Date.

Contrary to its name, Date objects are simply wrappers around a UTC unix timestamp. They only thing they do with timezones out of the box is mapping to the browsers timezone. The Date object itself has no timezone information at all, so it is generally not possible to handle time zones with it alone. Dayjs wraps these Date objects with some additional information, but is still pretty limited and buggy with time zones. It frequently does not handle DST properly (for instance iamkun/dayjs#1260).

The implementation approach taken in #3823 (which I have not had time to finish) attempts to resolve this pain point by introducing a timezone in the mantine context and mapping created Date objects from the browsers timezone to the given timezone (if it exists). It's error prone and hard to enforce that this is done everywhere correctly because every dayjs object has to be constructed from the context which is easy to forget. I still think this would be a great addition to the mantine dates library though due to its backwards compatibility.

The temporal API introduces several new distinct objects to handle the complexity involved. For instance, it has PlateDate for representing just one calendar day, PlainTime for just representing the time within a day, etc. It would be wonderful to eventually have input components for each of these specific types instead of using Date objects to awkwardly represent them. For example, TimeInput could use PlainTime, DateInput/DatePicket could use PlainDate, DateTimePicker could use PlainDateTime, etc.

As you say, it would also be possible to return dayjs objects directly, instead of Date. I tried this in my code base via wrapping components but found it unreliable because there isn't an easy way to know what kind of dayjs object you have (utc, in tz, etc). It does afford passing timezone information, but I'm not sure it's worth it in the long run because of the eventual introduction of the temporal API. As you say it would also be breaking if it is introduced by changing the existing components.

Since the temporal API is not production ready yet, I don't think it makes sense to introduce this to mantine proper yet. It's possible to create wrapping components however, and it may make sense to have a distinct library in the meantime that utilizes a polyfill. This is the direction I will likely take my code base and if I end up with something generally usable I will publish it as a distinct library.

@wuifdesign
Copy link
Contributor

wuifdesign commented Sep 5, 2023

@benlongo
Temporal API would be nice, but i think it will take a few years until all major browser will support it and you can use it (at least without polyfill). Currently i'm building an app where the user is able to choose his timezone which is not possible with mantine.

i haven't tried the wrapper approach, wich may work, but produces an overhead instead of just mantine supporting this.

the main problem is, that mantine uses the plane Date objects (not having timezone support yet) and only uses dayjs on some calculations inside.

You are right, temporal api would be a nice thing, but do you think it will be usable soon. i can't find anything about the release timeline of this.

@ArnaudValensi
Copy link
Author

One approach would be to use plain date strings as properties of the date picker and use dayjs internally.

@wuifdesign
Copy link
Contributor

@ArnaudValensi this would have been my approach, but it would stille be a breaking change as mantine exports helper functions like getMonthDays, getEndOfWeek, ... which also use the Date object, and it be really hard to convert them each time.

Exporting the date for the components would be possible, but doing this for all the helper function will be a bit too much i think. especially as the recreation of the dayjs objects will take calculation time too.

@benlongo
Copy link
Contributor

benlongo commented Sep 5, 2023

@ArnaudValensi I think the string approach as a baseline could work well. Various wrapping layers/components (Dayjs, Date, Temporal, etc.) could be created on-top of it. For backwards compat, the Date version can be exposed under the same namespace and the string variants could be exposed under a new one. To minimize maintenance burden on mantine, any other variants could be mantained out of repo.

@wuifdesign
Copy link
Contributor

wuifdesign commented Sep 12, 2023

i added a MR to the v7 repo, i hope this will work for you. i introduced some kind of date "shift" to handle timezones without any breaking changes. @rtivital

@wuifdesign
Copy link
Contributor

wuifdesign commented Sep 16, 2023

merged and released to 7.0.0-beta.6 rtivital/mantine-v7#59

@rtivital
Copy link
Member

This issue is closed for one of these reasons:

  • It was fixed in 7.0
  • It is no longer reproducible in 7.0
  • It is no longer applicable in 7.0
  • It does not have a reproduction

If you think that this issue was closed by mistake and it is still an issue in version 7.0, please reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants