-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Fix for Issue #4136, MAGETWO-53440 #14485
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
Fix for Issue #4136, MAGETWO-53440 #14485
Conversation
1f98ac3
to
15242c0
Compare
Added calendar initializaton for Conditional Rules when a rule is created for the 1st time
15242c0
to
cc1e013
Compare
Hi @orlangur, thank you for the review. |
Hi @orlangur, thank you for the review. |
Hi @vasilii-b. There's still a possibility to write text in the datepicker input. Check the following scenario: |
Thank you, @rogyar . Will have a look on that. |
@rogyar, a fix was added for the provided scenario. Thank you! |
lib/web/mage/calendar.js
Outdated
@@ -377,6 +377,9 @@ | |||
.addClass('v-middle') | |||
.text('') // Remove jQuery UI datepicker generated image | |||
.append('<span>' + pickerButtonText + '</span>'); | |||
|
|||
$(element).attr('autocomplete', 'off'); |
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.
Hi @vasilii-b. Thanks for the collaboration. This fix removed autocomplete in all scenarios. Are you sure that is the right approach?
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.
Hi @VladimirZaets,
here is a good point where we can have a long discussion, I believe.
Yes, I intended to disable the autocomplete for all scenarios and here is why.
It all started from the thing that the datepicker goal is to provide the user a possibility to select the date, not to write it by hand. The input I believe should be informative only, because the input is the date picker itself. I'd suggest making the datepicker input readonly at all.
Secondly, from the autocomplete there may come values in different format than is needed by the datepicker.
Let's say on Website A there is input with name date
that has format DD/MM/YYYY
, on Website B there is an input with the same name (date
) but the format is MM/DD/YYY
. So on Website A user set his desired date and when comes on Website B browser suggests him the previous date (from Website A) - which is wrong because of the date format.
Please let me know your thoughts on this.
Thank you!
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 will as product manager, because it more related to the produce manager that to developer.
Anyway, I think we should add the configuration option to manage this situation and provide a choice to using this component in each 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.
@VladimirZaets sounds reasonable. I'll add the additional configuration.
Thank you!
Hi @vasilii-b , I am closing this PR now due to inactivity. |
Hi @sidolov, |
Hi @VladimirZaets, thank you for the review. |
…acter not preventing from saving - Restricted user to change date input value - Open datepicker popup once user click on the date to be changed
71dca3d
to
6c87bb3
Compare
Hi @VladimirZaets, |
@VladimirZaets, please let me know if newly changes are fine for you. Thanks! |
…acter not preventing from saving - Added autocomplete option for calendar.js
Hey, what's the status of this one ? Accepted or need some more attention ? Thank you! |
Hi @vasilii-b. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
Description
Full description in #4136.
✔️ Added calendar initialization for Conditional Rules when a rule is created for the 1st time
For the condition type date the datepicker was not initialized when creating new conditions.
This PR aims to solve the issue and do not allow the user to write anything to the input.
Fixed Issues (if relevant)
Issue described in #4136
Manual testing scenarios
Described in #4136
Contribution checklist