Skip to content

Conversation

@bobemoe
Copy link
Contributor

@bobemoe bobemoe commented Jul 2, 2019

Original Pull Request

#23528

Description (*)

The issue fixed in #8298 made huge improvement however there is still a 1px gap where the nav functionality breaks. This is especially obvious on iPad where viewport is exactly 768px. This patch fixes the functionality by moving the breakpoint by 1px.

Fixed Issues (if relevant)

  1. Mobile Menu Behavior at Incorrect Breakpoint #8298

Manual testing scenarios (*)

  1. Clean install with sample data
  2. Open front page in a browser with specific width control (e.g firefox in responsive design mode)
  3. Set viewport width to 767: see main nav is hidden and mobile/burger menu is visible (GOOD)
  4. Set viewport width to 769: see main nav is visible and mobile/burger menu is hidden, dropdowns work on hover. (GOOD)
  5. Set viewport width to 768: see main nav is visible and mobile/burger menu is hidden (GOOD, but:)
    1. without this patch: at 768 the dropdowns do not work on hover, and they contain the "All {category}" items that are only meant to be added on the burger/mobile nav.
    2. with this patch: at 768 the nav functions as expected, dropdown on hover and not including the "All {category}" items.

Questions or comments

I do see 768 hard coded again in the menu.js file further down I tried changing this too but didn't see any affect. I don't know what that does or why its not using the mediaBreakpoint variable, so I left it alone.

Contribution checklist (*)

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

@m2-assistant
Copy link

m2-assistant bot commented Jul 2, 2019

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

For more details, please, review the Magento Contributor Assistant documentation

@ihor-sviziev ihor-sviziev self-assigned this Jul 3, 2019
@ihor-sviziev ihor-sviziev changed the title fix nav breakpoint issue (backports #23528) [Backport] move breakpoint by -1px to make nav work correctly at viewport 768 Jul 4, 2019
@ihor-sviziev ihor-sviziev added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Port and removed Progress: ready for testing labels Jul 4, 2019
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5394 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Jul 5, 2019

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

magento-engcom-team pushed a commit that referenced this pull request Jul 5, 2019
@magento-engcom-team magento-engcom-team added this to the Release: 2.2.10 milestone Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants