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

Feature suggestion: Timezone support for datetime() renderer #289

Open
xJuvi opened this issue Aug 7, 2024 · 5 comments · May be fixed by #291
Open

Feature suggestion: Timezone support for datetime() renderer #289

xJuvi opened this issue Aug 7, 2024 · 5 comments · May be fixed by #291

Comments

@xJuvi
Copy link

xJuvi commented Aug 7, 2024

Hey there,

i store all timestamps as unix timestamp in my database. When i display them in a form or other page, they will be displayed in UTC+2 (german summertime). Since a few weeks i doesn't convert the timestamps before i display them with datatables. But all timestamps will be displayed in UTC+0. So the time is two hours in the past.

I tried a look in the renderer source code.....

function __mldObj (d, format, locale) {
var dt;
resolveWindowLibs();
if (__moment) {
dt = __moment.utc( d, format, locale, true );
if (! dt.isValid()) {
return null;
}
}
else if (__luxon) {
dt = format && typeof d === 'string'
? __luxon.DateTime.fromFormat( d, format )
: __luxon.DateTime.fromISO( d );
if (! dt.isValid) {
return null;
}
dt.setLocale(locale);
}
else if (! format) {
// No format given, must be ISO
dt = new Date(d);

I think where wo use luxon or moment it was very easy to convert the time object in another timezone. The are several ways possible. As fourth parameter we can add the destination timezone. And if we want, we can also add a fifth parameter as current timezone.

Are there any plans for this? Is it allowed to make a short pull request to add this feature? I really need it. Otherwise the datetime renderer isn't useful for me and i need to write my own one. But my suggested changes are no breaking changes and maybe will help some other people.

Kind regards

@AllanJard
Copy link
Contributor

No plans for this at the moment, as I think you are the first to ask for it, but I think it is a fair request, and a PR would be welcome. There might be a number of nuances though, for example what if the date/time given doesn't include timezone information. Do we assume that it is UTC, or local time?

@xJuvi
Copy link
Author

xJuvi commented Aug 8, 2024

Difficult question. Thanks for you quick reply.
It only relates when no timezone information is added and a conversion should be done. In any other situation wie can only change the format like now.

I think the best way for timezone conversion is to use UTC as default. Then every user has the same result. Otherwise users with different settings have different results. So if you input isn't UTC and you want to convert the timezone, you must also add the current given timezone. Or do you have another opinion?

@AllanJard
Copy link
Contributor

I think that is probably fair. The results could be rather unpredictable otherwise.

So:

  • Input with no timezone + format with no timezone = treat both as UTC (i.e. no timezone offset applied)
  • Input with timezone + format with no timezone = ? Format using the timezone from the input, so no offset applied
  • Input with no timezone + format with timezone = Treat input as UTC
  • Input with timezone + format with timezone = Sanity

I'm a little worried about how much code this might add to the library, and there will need to be a good number of unit tests for it (the unit tests I think actually only pass in GMT timezone already at the moment - that's something I've been meaning to look into!).

In principle I think it is a good idea to handle this. It might not be as simple as passing a timezone as an extra parameter though. There is an argument to say that the middle two should be considered invalid inputs and shouldn't be considered... But I just know devs will feed it those combinations.

@xJuvi
Copy link
Author

xJuvi commented Aug 8, 2024

Ok well. That's so complicated. I read the source code again an checked the momentjs and luxon docu.

At this time you use this for momentjs. There, the given time is handled as UTC+0.

dt = __moment.utc( d, format, locale, true );

With this small optimization, momentjs handle the given time as local time:

dt = __moment( d, format, locale, true );

Luxon use the same handle but it's correct imlemented.

This small change solve my problem because all my users are live in germany. I think this change isn't a feature request more a bugfix. Did you agree?

To change the time to another timezone is bit more difficut because we can't use the logic from momentjs an luxon. Maybe it's a feature for a later release? With both libraries it isn't much code, but a good testig id needed.

If you confirm my small suggestion to remove the moment.utc() function against moment() i can create a fix.

EDIT: Here the link to the momentjs docu:
https://momentjs.com/docs/#/parsing/

@AllanJard
Copy link
Contributor

I think that is okay! I really need to add some unit tests to check. Feel free to send a PR and when I get a chance to write some tests, I'll merge it in.

@xJuvi xJuvi linked a pull request Aug 8, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants