-
Notifications
You must be signed in to change notification settings - Fork 17
Fix/broken datepicker #343
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
Conversation
|
|
||
| it('should update the model if the value is a valid date', () => { | ||
| const date = new Date(); | ||
| const date = new Date('2018-01-10T00:00:00Z'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would new Date() not suffice? Or in other words, is this implementation going to be a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done in order to have the same result every time the test ran. Changing this back to new Date() would not cause the test to fail. However, in regards to the breaking change: actually, yes, it is a breaking change, I will adjust the PR description.
Users of the component will now always get back the Brussels timezone value. So if there are applications that were doing their own calculations in order to get the correct value for their specific timezone, those will need to check their implementations of the calendar and datepicker components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, clear. In this case, I would not label it as a breaking change (in semantic versioning), unless the previous behaviour is fixed first in this major release. Agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it should be kept as a bug/fix, but I would add a clear explanation in the release notes and recommend users to thoroughly test their date calculations after upgrading
PR Checklist
This PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When selecting a date via the Datepicker component from a different timezone (e.g. Tokyo or Los Angeles), the display value of the input field, the actual value, and the "selected" (graphically) value in the calendar do not match.
Issue Number: N/A
What is the new behavior?
When selecting a date via the Datepicker component from any timezone, it will select and display the correct value.
Does this PR introduce a breaking change?
Other information
Updated the date-fns package to the latest version and uses the @date-fns/tz from this new version.
The current version of TypeScript for the angular components is not entirely compatible with this new version of the package, due to its use of the "export type *" syntax, .cts extensions and Intl types. Added skipLibCheck to tsconfig because of this, but for the future we should try to update the components so that they are compatible with newer versions of TypeScript.
Resolved issues
https://jira.antwerpen.be/browse/GFOP-162