-
Notifications
You must be signed in to change notification settings - Fork 45
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
datepicker timezone issue on view layer fixed #1168
Conversation
* We only test for 'yyyy-MM-dd' because timezone related issue only occurs when date is selected from calendar. | ||
* And calendar only sets value in yyyy-MM-dd | ||
*/ | ||
if (isDateDefaultFormat(value)) { |
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.
Date will always come in this format, why have you added this check ? We don't support any other format in saveValue.
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.
If a user manually enters some other date format from the input widget
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.
For saveValue there is no formatting, you don't need this check. Formatting is only for display/edit value
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1168 +/- ##
=========================================
Coverage 80.75% 80.75%
Complexity 773 773
=========================================
Files 91 91
Lines 2110 2110
Branches 285 285
=========================================
Hits 1704 1704
Misses 252 252
Partials 154 154 ☔ View full report in Codecov by Sentry. |
@@ -1119,10 +1119,6 @@ if (typeof window.DatePickerWidget === 'undefined') { | |||
if (this.#isEditValueOrDisplayValue(this.#widget.value)) { | |||
return this.#model.value; // if the widget has edit/display value thus method should return model value | |||
} | |||
let dateVal = new Date(this.toString()); | |||
if (!isNaN(dateVal)) { |
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 was this added earlier?
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
@@ -1119,10 +1119,6 @@ if (typeof window.DatePickerWidget === 'undefined') { | |||
if (this.#isEditValueOrDisplayValue(this.#widget.value)) { | |||
return this.#model.value; // if the widget has edit/display value thus method should return model value | |||
} | |||
let dateVal = new Date(this.toString()); |
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 don't think this should be removed
datepicker timezone issue on view layer fixed datepicker timezone issue on view layer fixed
6484722
to
74bbd69
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
@@ -99,6 +99,12 @@ | |||
this.widgetObject.setValue(e.target.value); | |||
this.setActive(); | |||
}, this.getWidget()); | |||
this.widgetObject.addEventListener('input', (e) => { |
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 am not sure how is this change related to the current issue.
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 issue is not directly related to timezone issue, but when we fix the timezone, this issue manifests in one of the test case.
Reason being that we were not clearing the value of the input field when user emptied it. (We do that from calendar icon but I think this was missed when manual entry of date was implemented).
@@ -1194,6 +1197,9 @@ if (typeof window.DatePickerWidget === 'undefined') { | |||
case 'focus': | |||
handler(e); | |||
break; | |||
case 'input': |
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 do you need this listener in the current CL? Please fix this as a separate JIRA
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.
PR will not pass if this is not fixed, or will have to comment out a test case
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.
check comments
* datepicker timezone issue on view layer fixed datepicker timezone issue on view layer fixed datepicker timezone issue on view layer fixed * datepicker fix test case
* datepicker timezone issue on view layer fixed datepicker timezone issue on view layer fixed datepicker timezone issue on view layer fixed * datepicker fix test case
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: