Remove over complicated setup for closing minicart dropdown #28906

lumnn · 2020-06-26T19:52:58Z

Description (*)

When investigating minicart code I've noticed a bit strange way to add event listener to elements that are supposed to close minicart when clicked.

Please first look on removed line with data-bind="visible: closeSidebar()". visible knockout binding is used to control whether the element should be shown. However, the closeSidebar function is always returning true value. The side effect of this function is it setup an event listeners to [data-action="close"] elements, which will close the minicart dropdown.

This pull request removes closeSidebar function and the knockout binding on completely unrelated element. Instead, the appropriate click binding is added to the only [data-action="close"] element I've found.

Manual testing scenarios (*)

Open minibasket
Close with an "X" button
Add a product to basket
Open, close minibasket
Open basket and click on "Proceed to Checkout"

Questions or comments

I've searched the codebase for elements with data-action="close" to ensure that it will not break any other functionality. However I'm guessing some modules may using this "API".

Other question is whether data-action="close" attribute is needed. By searching in a code it doesn't seem to have a purpose, but it increase HTML to content ratio.

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 are green)

Resolved issues:

resolves [Issue] Remove over complicated setup for closing minicart dropdown #29161: Remove over complicated setup for closing minicart dropdown

m2-assistant · 2020-06-26T19:53:00Z

Hi @lumnn. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

lbajsarowicz · 2020-06-27T00:33:19Z

@magento run all tests

Very interesting change. Let's make sure that everything works fine!

lumnn · 2020-06-27T07:39:38Z

I'm unsure why tests fails as from the reports I can't find that any of the issue is related to the changes I've done

@magento run Functional Tests CE , Functional Tests B2B

lumnn · 2020-06-27T10:21:42Z

I guess it's worth noting that Functional Tests CE and Functional Tests B2B failed in the first go, but passed in second.

lbajsarowicz

✔️ Wow. Great work! Thank you for your contribution.

magento-engcom-team · 2020-06-27T20:51:47Z

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7752 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

magento-engcom-team · 2020-06-27T20:51:48Z

@lumnn thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

magento-engcom-team · 2020-06-28T21:05:48Z

Hi @ptylek, thank you for the review.
ENGCOM-7752 has been created to process this Pull Request
✳️ @ptylek, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

ptylek · 2020-07-11T19:16:19Z

Adding not required label as this PR improves on some redundant javascript. Functionality should stay the same.

engcom-Delta · 2020-07-13T10:34:16Z

✔️ QA passed
Was checked that behavior minicart "X" button is same as before for Mobile, Tablet, Desktop views

Mobile
Tablet
Desktop

VladimirZaets · 2020-07-15T16:18:27Z

@magento create issue

…own #28906

m2-assistant · 2020-07-29T08:28:04Z

Hi @lumnn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Remove over complicated setup for closing minicart

1cb5685

ghost added the Progress: pending review label Jun 26, 2020

magento-engcom-team added Area: Frontend Component: Checkout Release Line: 2.4 labels Jun 26, 2020

lumnn marked this pull request as ready for review June 26, 2020 20:11

lbajsarowicz approved these changes Jun 27, 2020

View reviewed changes

ghost assigned lbajsarowicz Jun 27, 2020

ghost added Progress: ready for testing and removed Progress: pending review labels Jun 27, 2020

ptylek approved these changes Jun 28, 2020

View reviewed changes

ghost assigned ptylek Jun 28, 2020

ptylek added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Jul 11, 2020

ptylek added the Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. label Jul 11, 2020

engcom-Delta self-assigned this Jul 13, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Jul 13, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Jul 13, 2020

engcom-Echo self-assigned this Jul 13, 2020

engcom-Echo added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Jul 13, 2020

m2-assistant bot mentioned this pull request Jul 15, 2020

[Issue] Remove over complicated setup for closing minicart dropdown #29161

Closed

4 tasks

ghost added the Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. label Jul 15, 2020

engcom-Delta added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Jul 24, 2020

ghost added Progress: accept and removed Progress: extended testing labels Jul 28, 2020

magento-engcom-team pushed a commit that referenced this pull request Jul 29, 2020

ENGCOM-7752: Remove over complicated setup for closing minicart dropd…

13d2cfa

…own #28906

magento-engcom-team merged commit 4f7e659 into magento:2.4-develop Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove over complicated setup for closing minicart dropdown #28906

Remove over complicated setup for closing minicart dropdown #28906

Uh oh!

lumnn commented Jun 26, 2020 •

edited by m2-assistant bot

Loading

Uh oh!

m2-assistant bot commented Jun 26, 2020

Uh oh!

lbajsarowicz commented Jun 27, 2020

Uh oh!

lumnn commented Jun 27, 2020 •

edited

Loading

Uh oh!

lumnn commented Jun 27, 2020

Uh oh!

lbajsarowicz left a comment

Uh oh!

magento-engcom-team commented Jun 27, 2020

Uh oh!

magento-engcom-team commented Jun 27, 2020

Uh oh!

magento-engcom-team commented Jun 28, 2020

Uh oh!

ptylek commented Jul 11, 2020

Uh oh!

engcom-Delta commented Jul 13, 2020

Uh oh!

VladimirZaets commented Jul 15, 2020

Uh oh!

m2-assistant bot commented Jul 29, 2020

Uh oh!

Uh oh!

Remove over complicated setup for closing minicart dropdown #28906

Remove over complicated setup for closing minicart dropdown #28906

Uh oh!

Conversation

lumnn commented Jun 26, 2020 • edited by m2-assistant bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

Uh oh!

m2-assistant bot commented Jun 26, 2020

Uh oh!

lbajsarowicz commented Jun 27, 2020

Uh oh!

lumnn commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lumnn commented Jun 27, 2020

Uh oh!

lbajsarowicz left a comment

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Jun 27, 2020

Uh oh!

magento-engcom-team commented Jun 27, 2020

Uh oh!

magento-engcom-team commented Jun 28, 2020

Uh oh!

ptylek commented Jul 11, 2020

Uh oh!

engcom-Delta commented Jul 13, 2020

Uh oh!

VladimirZaets commented Jul 15, 2020

Uh oh!

m2-assistant bot commented Jul 29, 2020

Uh oh!

Uh oh!

lumnn commented Jun 26, 2020 •

edited by m2-assistant bot

Loading

lumnn commented Jun 27, 2020 •

edited

Loading