Follow the mobile menu state instead of flipping the header class - #227
Draft
boo-code wants to merge 1 commit into
Draft
Follow the mobile menu state instead of flipping the header class#227boo-code wants to merge 1 commit into
boo-code wants to merge 1 commit into
Conversation
toggleMobileMenu() is called from the menu button and from the 'responsive update' event, which fires on every breakpoint crossing. A crossing opens and closes nothing, so the unconditional toggleClass left #header.is-open inverted with respect to the menu until the next crossing put it back. Read #mobile_top_menu_wrapper's visibility once and drive both the class and the content hiding from it, as the content hiding already did. See PrestaShop/PrestaShop#35939
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TopMenu.toggleMobileMenu()has two callers: the#menu-iconclick, where a toggle is right because#mobile_top_menu_wrapperwas just toggled, andprestashop.on('responsive update'), which fires on every crossing of the 768px breakpoint. A crossing opens and closes nothing, so the unconditional$('#header').toggleClass('is-open')flips the class for free.The consequence is worse than a stray class. After an odd number of crossings
is-openmeans the opposite of the menu state, and every later click preserves that inversion until the next crossing puts it back. The hide/show of#notifications, #wrapper, #footerin the same method was always driven by the wrapper's real visibility; the class is now driven by the same value.#headerin the inspector, and drag the window across 768px without touching the menu. Before this changeis-openappears on the way down and disappears on the way up; after it, it appears only when the mobile menu is actually open.The sharper check is the phase: at mobile width, cross the breakpoint once, then open and close the menu. Before, the class is present while the menu is closed and absent while it is open.
Measured
Real browser, real window resizes, the theme's own compiled bundle.
menuOpenis#mobile_top_menu_wrappervisibility,isOpenis#header.is-open.Fresh load already at mobile width, no crossing — this is the control, and it agrees:
Loaded at desktop, then resized down across 768px — one crossing:
Three positions out of three inverted, and the only difference from the control is the crossing.
Restoring the unpatched bundle brings all three
NOs back, with the served file checked fortoggleClass("is-open")versustoggleClass("is-open",in the same call that reads the table.Menu left open at mobile width, then resized up: after the fix the class is removed and the page
content is restored,
agree: yes.Why the load path is quiet
responsive.jsalso emitsresponsive updatefrom its own$(document).readywhen the page loadsalready narrow, which would toggle the class on load too. It does not, because that
readycallbackis registered at import time (
theme.js:33) and so runs before the one that constructsTopMenu(
theme.js:61) — at the moment of the load-time emit there is no listener. That is why the reportersees a clean class on a fresh mobile load and a wrong one only after resizing, and the control run
above confirms it.
Notes
is-openoccurs once in the whole theme, thetoggleClassitself,and zero times in its SCSS or compiled CSS. It exists to tell child themes and third-party scripts
whether the mobile menu is open, which is exactly the contract the crossing breaks.
assets/is gitignored in this repo, so no rebuilt bundle belongs in the diff. The build was runonly to produce the numbers above (
npm run buildin_dev, exit 0;npm run linton the file,exit 0).
overriding or calling
toggleMobileMenu()is unaffected.