feat(material-luxon-adapter) add option to set zone - #33579
Conversation
d3acb55 to
905f8f2
Compare
| * Changing this will change how Angular Material components like DatePicker output dates. | ||
| * The zone parameter will be ignored if the useUtc parameter is set to true. | ||
| */ | ||
| zone?: string; |
There was a problem hiding this comment.
I went with luxon's terminology here, but i'm totally fine with modifying it to timeZone. Should we stay with luxon's terminology or should i update it to timeZone?
| const result = this._useUTC | ||
| ? LuxonDateTime.utc(year, month + 1, date, options) | ||
| : LuxonDateTime.local(year, month + 1, date, options); | ||
| ? LuxonDateTime.utc(year, month + 1, date, this._getLocaleOptions()) |
There was a problem hiding this comment.
Doesn't setting the timezone conflict with the setting for enabling UTC?
There was a problem hiding this comment.
I didn't want to introduce any breaking change with this PR, so I kept the useUTC parameter and it has precedence over the new zone setting.
I added a comment to the parameter too: "The zone parameter will be ignored if the useUtc parameter is set to true.".
There was a problem hiding this comment.
We should throw an error if both UTC and timeZone are set. It's not a breaking change since there shouldn't be any apps setting the time zone at the moment.
| * Changing this will change how Angular Material components like DatePicker output dates. | ||
| * The zone parameter will be ignored if the useUtc parameter is set to true. | ||
| */ | ||
| setZone(zone?: string) { |
There was a problem hiding this comment.
I went with luxon's terminology here, but i'm totally fine with modifying it to timeZone. Should we stay with luxon's terminology or should i update it to setTimeZone (or setTimezone as you suggested)?
There was a problem hiding this comment.
@crisbeto should i go with setTimeZone or setTimezone as you suggested? If the parameter is timeZone, I guess it would be better to go with setTimeZone.
429e390 to
1560c63
Compare
|
@crisbeto I modifed the PR based on your suggestions. I also created some extra tests to test the error thrown when useUtc and timeZone is also provided. |
Add zone option to be able to change the zone through MAT_LUXON_DATE_ADAPTER_OPTIONS.
1560c63 to
1c2ff17
Compare
crisbeto
added
action: merge
|
This PR was merged into the repository. The changes were merged into the following branches:
|
2 participants
Add zone option to be able to change the zone through MAT_LUXON_DATE_ADAPTER_OPTIONS.
In a web app where the user can set their preferred timezone apart from local and UTC, the current implementation is not sufficient. The current UTC/local works fine, but there are use cases when it is not enough.
I added a new zone option to the MatLuxonDateAdapterOptions and it is passed everywhere in the adapter.
The main issue was with the DateRangePicker. However I gave it DateTimes that were in the desired timezone, after changing the range, the new DateTime objects were in local timezone.
Also, the today highlight was wrong with the today date that was using local timezone.
Basically the change is this in the _getOptions and some minor refactorings:
->
I also added the ability to configure the zone programmatically with a public
.setZone(zone: string)The DateTimeOptions was passed to every method. I refactored it a bit so the methods that are only accepting the LocaleOptions only receive the LocaleOptions, and pass only the DateTimeOptions where the luxon methods are accepting it. This makes it clearer which methods are working with the zone and which are not. So I created a
_getLocaleOptionsand a_getDateTimeOptionsinstead of the current_getOptions. I hope you don't mind, it seems clearer to me this way.While writing the tests, I copied the UTC tests and tried to make it work with the new zone setting. One of my tests was failing and it turned out that the original test was not working correctly. I fixed that too ("should parse dates to UTC"). The date format was wrong and the parse gave back null and the test basically tested if null equals null.
I also added some remarks to the documentation where it talked about the useUTC parameter.