Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix minicart promotion region not rendering #25373 #25375

Merged
merged 5 commits into from
Jan 17, 2020
Merged

Fix minicart promotion region not rendering #25373 #25375

merged 5 commits into from
Jan 17, 2020

Conversation

mattijv
Copy link
Contributor

@mattijv mattijv commented Oct 30, 2019

The minicart promotion region was not rendering due to a wrongly written check for the number of items in the 'promotion' region. The code checked for the length property of the observable returned from the getRegion method instead of the array it contained.

A fast way would have been to add the missing parentheses to the check (and I made antoher PR doing just that), but I felt that the getRegion(region)().length pattern is a bit hard to read and easy to make a mistake with.

Description (*)

This PR adds a new method to the lib/core/collection.js component of the Magento_Ui module that can be used to check if a region has any elements associated with it. This commit also replaces all occurences of getRegion(region)().length with a call to the new method.

Fixed Issues (if relevant)

  1. The 'promotion' region of the minicart is never rendered #25373: The 'promotion' region of the minicart is never rendered

Manual testing scenarios (*)

See related issue.

Questions or comments

This PR is an alternative to #25374. The fix implement in this PR is more involved, but I feel it improves to quality of Magento code and helps avoid similar mistakes in the future.

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)

@mattijv
Fix minicart promotion region not rendering
20d3f40 
The minicart promotion region was not rendering due to
a wrongly written check for the number of items in the
'promotion' region. The code checked for the length
property of the observable returned from the getRegion
method instead of the array it contained.

A fast way would have been to add the missing parentheses
to the check (and I made antoher PR doing just that), but
I felt that the getRegion(region)().length pattern is a
bit hard to read and easy to make a mistake with.

Therefore, this commit adds a new method to the
lib/core/collection.js component of the Magento_Ui module
that can be used to check if a region has any elements
associated with it. This commit also replaces all occurences
of getRegion(region)().length with a call to the new method.
@m2-assistant
Copy link

m2-assistant bot commented Oct 30, 2019

Hi @mattijv. 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.3-develop instance - deploy vanilla Magento instance

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

This was referenced Oct 30, 2019
Closed
Closed
@mattijv mattijv closed this Oct 30, 2019
@m2-assistant
Copy link

m2-assistant bot commented Oct 30, 2019

Hi @mattijv, 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.

@mattijv mattijv reopened this Oct 30, 2019
@mattijv
Copy link
Contributor Author

mattijv commented Oct 30, 2019

Apparently there is some latency to the CLA signing being registered? I'm going to try closing and reopening this PR later.

@mattijv mattijv closed this Nov 1, 2019
@m2-assistant
Copy link

m2-assistant bot commented Nov 1, 2019

Hi @mattijv, 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.

@mattijv mattijv reopened this Nov 1, 2019
@mattijv
Add missing newline after var declaration
9b6c8d7 
This is to appease the code style checker.
@mattijv
Copy link
Contributor Author

mattijv commented Nov 4, 2019

As far as I can tell, all the remaining failing tests are unrelated to any changes in this PR.

krzksz
krzksz approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@krzksz krzksz left a comment

Choose a reason for hiding this comment

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

The suffix check was introduced in a303ce8 and I agree adjustments to comply with it should be done in a separate PR.

@Karlasa Karlasa self-assigned this Nov 8, 2019
@Karlasa Karlasa added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Nov 8, 2019
@engcom-Alfa engcom-Alfa self-assigned this Nov 8, 2019
@engcom-Alfa
Copy link
Contributor

@mattijv Thank you for the clarification!

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@engcom-Foxtrot engcom-Foxtrot self-assigned this Nov 12, 2019
@sdzhepa
Copy link
Contributor

sdzhepa commented Nov 12, 2019

@magento run all tests

@mattijv
Copy link
Contributor Author

mattijv commented Nov 13, 2019

As it seems this PR is tentatively approved as the proper fix, I'll close #25374 as redundant.

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:16
@VladimirZaets
Copy link
Contributor

B2B tests are failed

@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

magento-engcom-team pushed a commit that referenced this pull request Jan 17, 2020
@VladimirZaets
ENGCOM-6251: Fix minicart promotion region not rendering #25373 #25375
b32567b
@magento-engcom-team magento-engcom-team merged commit 109392d into magento:2.4-develop Jan 17, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jan 17, 2020

Hi @mattijv, 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.

@mrtuvn mrtuvn mentioned this pull request Jun 9, 2020
Closed
4 tasks
@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Karlasa Karlasa Karlasa approved these changes

@krzksz krzksz krzksz approved these changes

@akaplya akaplya Awaiting requested review from akaplya

@DrewML DrewML Awaiting requested review from DrewML

@paliarush paliarush Awaiting requested review from paliarush

Assignees

@Karlasa Karlasa

@VladimirZaets VladimirZaets

@engcom-Alfa engcom-Alfa

@engcom-Foxtrot engcom-Foxtrot

Labels
Area: Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix Component: Catalog Component: Checkout Component: Ui Progress: accept Release Line: 2.4 Squashtoberfest 2019
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mattijv @engcom-Alfa @sdzhepa @VladimirZaets @engcom-Foxtrot @krzksz @Karlasa @sidolov @magento-engcom-team