Skip to content

[Forwardport] Fix for Issue #4136, MAGETWO-53440 #14330

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

Closed
wants to merge 1 commit into from

Conversation

vasilii-b
Copy link

@vasilii-b vasilii-b commented Mar 24, 2018

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

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

Original PR

#14485

@orlangur orlangur self-assigned this Mar 24, 2018
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Please change implementation in such a way that calendar is successfully initialized via data-mage-init (so that whenever you add it to DOM it is auto-wired).

@vasilii-b
Copy link
Author

vasilii-b commented Mar 24, 2018

@orlangur I'm not sure if that's possible. Tough you may suggest me where (what module, file) does the parsing of the data-mage-init so I can investigate how to solve this in a better way.
As far as I know the data-mage-init is processed once the DOM is loaded and does not listen to any other DOM events. Please correct me if I'm wrong.

Thank you!

@vasilii-b
Copy link
Author

@orlangur nevermind, I figured it out. Please check the latest commit. Thank you!

@@ -220,6 +221,8 @@ define([

var elem = Element.down(elemContainer, 'input.input-text');

jQuery(elem).trigger('contentUpdated');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly 👍

Looks like calendar dependency is not needed anymore, isn't it? Please squash changes into a single commit also.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I've removed it. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter too, please: +], function (jQuery, calendar) {

If you need any assistance with squashing, just tell me.

@vasilii-b
Copy link
Author

@orlangur changes added

@orlangur
Copy link
Contributor

orlangur commented Apr 1, 2018

@vasilii-b great! Changes look good to me now, there is one thing I didn't mention before - this PR is targeting 2.3-develop while we should deliver every contribution into 2.2-develop first.

Could you please apply similar changes to 2.2-develop in separate PR and I'll process them both instantly then.

@vasilii-b
Copy link
Author

@orlangur please see separated request #14485
Thank you a lot for help!

@vasilii-b
Copy link
Author

Hi @orlangur, can you please update me why this is on on hold ?

@vasilii-b vasilii-b changed the title Fix for Issue #4136, MAGETWO-53440 [Forwardport] Fix for Issue #4136, MAGETWO-53440 Jun 22, 2018
@ishakhsuvarov ishakhsuvarov added this to the Release: 2.3.1 milestone Sep 18, 2018
@sidolov
Copy link
Contributor

sidolov commented Sep 20, 2018

Hi @vasilii-b , I'm closing this PR because original PR was closed due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Sep 20, 2018
@vasilii-b vasilii-b deleted the issue-4136 branch November 1, 2018 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants