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

Accept ISO 8601 Date-only string as input for IgxDateTimeEditorDirective #6994

Closed
pmoleri opened this issue Mar 27, 2020 · 9 comments · Fixed by #9160
Closed

Accept ISO 8601 Date-only string as input for IgxDateTimeEditorDirective #6994

pmoleri opened this issue Mar 27, 2020 · 9 comments · Fixed by #9160

Comments

@pmoleri
Copy link

pmoleri commented Mar 27, 2020

Problem

Calendar and DatePicker use Date class. Date class represents a timestamp which is often troublesome to handle date-only values.
OpenAPI (swagger) defines format date to be a date-only string (e.g. 2017-07-21).
Currently the Calendar doesn't allow to use information from a Rest endpoint, conforming to the Swagger date format, to display a birthday calendar, or and date picker like <input type="date" /> allows.
I need to first create Date instances time-shifted to my local time so Calendar is able to display them correctly. The same will happen for other concepts as Holidays, target release date, etc.
e.g. new Date('2020-03-27') -> 2020-03-26 21:00:00 GMT-0300 (this makes the Calendar display the wrong date).

Describe the solution you'd like

Calendar and DatePicker could accept ISO 8601 dates. To internally convert those strings to Date objects or not is an implementation decision, if it's converted it should parse it in a way that it's interpreted as local time, but this should be transparent to the user.
It should also be possible to subscribe to changes using ISO 8601 Date strings, e.g. currently calendar onSelection emits Date objects.

Describe alternatives you've considered

Create a pipe to transform the date-only strings to local Dates

return new Date('2020-03-27' + 'T00:00:00'); // adding time with no timezone makes it parse it as local time.

When subscribing to changes I would need to do a conversion to serialize back to date-only strings.
The pipe (input) could be provided by igniteui to use optionally, but this wouldn't solve the subscribe (output) issue.

Additional context

HTML <input type="date" /> accepts and emits ISO Date-only strings, just like the swagger format.
By implementing this feature, it allows to switch between <input type="date"> and igx-datepicker, while using the same standard format for values.

Related issues: #8256

@github-actions
Copy link

There has been no recent activity and this issue has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label May 27, 2020
@Lipata Lipata removed the status: inactive Used to stale issues and pull requests label May 27, 2020
@zdrawku zdrawku removed their assignment Jun 17, 2020
@Lipata Lipata assigned Lipata and jackofdiamond5 and unassigned Lipata Jun 19, 2020
@Lipata Lipata added the size: L label Aug 11, 2020
@jackofdiamond5
Copy link
Member

We have a plan to refactor our date picker and time picker components where we will integrate our IgxDateTimeEditorDirective (#6483, #6482). It will be responsible for all input handling and it will also support ISO strings, thus both pickers will have the functionality available to them. This means that we can relate this issue to our editor directive, instead.

@jackofdiamond5 jackofdiamond5 changed the title Accept ISO 8601 Date-only string as input for igx-calendar and igx-datepicker Accept ISO 8601 Date-only string as input for IgxDateTimeEditorDirective Aug 13, 2020
@github-actions
Copy link

There has been no recent activity and this issue has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Oct 13, 2020
@Lipata Lipata removed the status: inactive Used to stale issues and pull requests label Oct 13, 2020
@github-actions
Copy link

There has been no recent activity and this issue has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Dec 13, 2020
@jackofdiamond5 jackofdiamond5 removed the status: inactive Used to stale issues and pull requests label Dec 13, 2020
@github-actions
Copy link

There has been no recent activity and this issue has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Feb 12, 2021
@Lipata Lipata removed the status: inactive Used to stale issues and pull requests label Feb 12, 2021
@jackofdiamond5
Copy link
Member

@pmoleri Our IgxDateTimeEditorDirective is to be integrated in the IgxDatePickerComponent. It will support ISO 8601 strings, however, there are a few things to be noted.

We will be handling ISO strings the way the browser does and as such, time shifts are to be expected in date only strings since they are defined as UTC according to the ecma spec.
Additionally, we want to be consistent between different frameworks for our toolkit and, for example, there are inconsistencies between how Angular handle bound strings to their components vs how their date pipe does it sample. That is mainly because they have custom logic for their date pipe, while bound dates are handled the same way the browser does it.

This means that any date only string that is bound to the editor will be directly given to the Date object which will treat it in local time. Subsequently the valueChange event (used for two-way binding) will emit a Date object - the now-parsed date only string. In order to escape from having to deal with time shifts, it will be required to pass in full ISO strings.

Please, do share your thoughts.

@pmoleri
Copy link
Author

pmoleri commented Mar 30, 2021

So, my thoughts are:

JS doesn't really support Date-only, it's always a timestamp. If I store a date-only string is because I care about that specific date (like birthday, anniversary, holiday, etc), so IMO time-shift is not an option. I'm in GMT-3, meaning that time-shift will actually change the date.

I think supporting date-only strings is an improvement on Calendar / Date Picker. And, I think it should be encouraged, as date-only string is a much better representation of a Day than the JS Date object (which is actually a UTC timestamp).

PS: In the future, when temporal API is standarized, we will be able to use PlainDate objects instead of date-only strings.

@jackofdiamond5 jackofdiamond5 added 🛠️ status: in-development Issues and PRs with active development on them and removed 🆕 status: new labels Apr 1, 2021
@jackofdiamond5
Copy link
Member

@pmoleri, we had a meeting two days back with @Lipata, @damyanpetev, @kdinev and @rkaraivanov about this and there it was decided that we don't want to fully support date only strings as described in this issue. We want our editors to handle dates the way the browser does it and to only emit date objects.

If we support date only strings and allow bindings to them then we will also have to consider what to return to the user. For example, if you have a bound string '2020-03' (which is a valid ISO string) then should we emit the value in the exact same format as it was provided or should we append a default value of, say, "-01"? This brings another problem, if we emit the value in the same format then some information may be lost. Because if you initially have '2020-03' and update the date segment of that string which should be present, then the emitter will only return '2020-03', no matter how many times you update it. And you can update all portions of that value because the editors use Date objects internally, which can be modified through the API quite easily. On the other hand, if we append something to that value, to "normalize" it, then this changes the initial format which I think is a no-go as well.

Also, the workaround with "T00:00:00" seems intended because according to the ecma spec if we have a date only string it is interpreted as a UTC time while strings with a specified time portion are treated as local time.

@pmoleri
Copy link
Author

pmoleri commented Apr 1, 2021

We want our editors to handle dates the way the browser does it and to only emit date objects

Not sure what this means. This is the opposite to what <input type="date"> does.

If you initially have '2020-03' and update the date segment of that string which should be present, then the emitter will only return '2020-03', no matter how many times you update it.

That's not a valid date-only string, that's a string representing a month. So, it's valid for a month picker, not a date picker. In date picker, if you are to support this, you should return standard iso date-only string like <input> does. I don't see a benefit in trying to mimic a non-standard provided value.

According to the ecma spec if we have a date only string it is interpreted as a UTC

Yes, ECMA spec is a mess, and they are working towards fixing it. Why would we take that as an oracle of good date representation say in an API or a database? Do I need to mangle standard strings to accommodate for Ecma and Ignite?

This is my feedback, it's no blocking for us. I just think that switching from standard <input> to Ignite shouldn't make things harder.

@Lipata Lipata added ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged. and removed 🛠️ status: in-development Issues and PRs with active development on them labels Apr 9, 2021
@Lipata Lipata mentioned this issue Apr 14, 2021
14 tasks
Lipata pushed a commit that referenced this issue Apr 21, 2021
…#6483 (#9160)

* feat(*): add PickersBaseDirective

* feat(date-time-editor): handling for wheel events

* feat(types): add new enum HeaderOrientation

* feat(date-time-editor): add preventSpeenOnWheel input

* feat(date-time-editor): can be set to suppress focus

* feat(DatePickerUtils): add method for min/max validation

* refactor(CalendarContainer): move to date-utils

* feat(PickersBaseDirective): implements EditorProvider

* refactor(picker-icons): move picker icon components to date-common

* feat(date-time-editor): add option to set spin delta per part #7169 (#8987)

* refactor(date-range): pickers base, overlay service, template buttons, valueChange

* refactor(date-editor): rename isSpinLoop to spinLoop

* feat(date-time-editor, date-range-picker, date-picker): ISO 8601 support #6994

* feat(date-picker): add readonly prop, close on escape

* refactor(advanced-filtering): update date picker template

* refactor(filtering-row): update date picker template

* refactor(excel-style-date-expr): update date picker template

* refactor(input-directive):  set disabled using hostbinding

* feat(igx-time-picker): refactoring #6482 (#8947)

* feat(date-time-editor, pickers): add migrations, changelog, readme #6482, #6483 (#9319)

Co-authored-by: Boris <jackofdiamond596@gmail.com>
Co-authored-by: plamenamiteva <plamenamiteva@abv.bg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants