[FIX] website: make anchor links work in offcanvas mobile menu #146132
Conversation
This file contains 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
|
Alternative mentioned in commit message would be about just this line if the Offcanvas.getInstance(offcanvasEl).hide(); |
|
|
rdeodoo
force-pushed
the
17.0-fix-offcanvas-mobile-anchor-rde
branch
from
3d99df3 to
63cfea3
Compare
rdeodoo
force-pushed
the
17.0-fix-offcanvas-mobile-anchor-rde
branch
3 times, most recently
from
f59769f to
423cf77
Compare
|
@qsm-odoo opw PR ready, feel free to delegate it should you prefer |
qsm-odoo
requested changes
Dec 14, 2023
rdeodoo
force-pushed
the
17.0-fix-offcanvas-mobile-anchor-rde
branch
from
423cf77 to
5413854
Compare
C3POdoo
requested review from
a team,
FrancoisGe and
BastienFafchamps
and removed request for
a team
|
I'll retarget to 16, here is the diff needed where offcanvas was introduced (probably 17) diff --git a/addons/web/tooling/_eslintrc.json b/addons/web/tooling/_eslintrc.json
index 467b2ff2ab785..016030f9cf324 100644
--- a/addons/web/tooling/_eslintrc.json
+++ b/addons/web/tooling/_eslintrc.json
@@ -43,24 +43,26 @@
"_": "readonly",
"Chart": "readonly",
"fuzzy": "readonly",
- "Popover": "readonly",
"StackTrace": "readonly",
"QUnit": "readonly",
"luxon": "readonly",
"py": "readonly",
"FullCalendar": "readonly",
"globalThis": "readonly",
- "Modal": "readonly",
- "Dropdown": "readonly",
"ScrollSpy": "readonly",
- "Tooltip": "readonly",
- "Collapse": "readonly",
- "Alert": "readonly",
"module": "readonly",
"chai": "readonly",
"describe": "readonly",
"it": "readonly",
"mocha": "readonly",
- "DOMPurify": "readonly"
+ "DOMPurify": "readonly",
+
+ "Alert": "readonly",
+ "Collapse": "readonly",
+ "Dropdown": "readonly",
+ "Modal": "readonly",
+ "Offcanvas": "readonly",
+ "Popover": "readonly",
+ "Tooltip": "readonly"
}
}
diff --git a/addons/website/static/src/js/content/snippets.animation.js b/addons/website/static/src/js/content/snippets.animation.js
index d1203c79de4ac..c4541e0d2bddd 100644
--- a/addons/website/static/src/js/content/snippets.animation.js
+++ b/addons/website/static/src/js/content/snippets.animation.js
@@ -1039,8 +1039,23 @@ registry.anchorSlide = publicWidget.Widget.extend({
if (!$anchor.length || !scrollValue) {
return;
}
- ev.preventDefault();
- this._scrollTo($anchor, scrollValue);
+
+ const offcanvasEl = document.querySelector('.offcanvas.o_navbar_mobile');
+ if (offcanvasEl && offcanvasEl.classList.contains('show')) {
+ // Special case for anchors in offcanvas in mobile: we can't just
+ // _scrollTo() after preventDefault because preventDefault would
+ // prevent the offcanvas to be closed. The choice is then to close
+ // it ourselves manually and once it's fully closed, then start our
+ // own smooth scrolling.
+ ev.preventDefault();
+ Offcanvas.getInstance(offcanvasEl).hide();
+ offcanvasEl.addEventListener('hidden.bs.offcanvas', () => {
+ this._scrollTo($anchor, scrollValue);
+ });
+ } else {
+ ev.preventDefault();
+ this._scrollTo($anchor, scrollValue);
+ }
},
});Commit msg |
rdeodoo
force-pushed
the
17.0-fix-offcanvas-mobile-anchor-rde
branch
from
5413854 to
8ef0cb0
Compare
|
@qsm-odoo this is ready, retargeted 16.0 as discussed here: #146132 (comment) |
qsm-odoo
force-pushed
the
17.0-fix-offcanvas-mobile-anchor-rde
branch
from
8ef0cb0 to
d511d91
Compare
A anchor menu in the mobile collapse related to an element in the current page doesn't work correctly: - The collapse-menu doesn't close - If the menu is large enough, on mobile, you actually don't even see the page scrolling behind it so it looks like the click did nothing. This is probably because of the switch from BS4 to BS5. In later version, it's even worse, as we are using bootstrap Offcanvas instead of collapse, creating another bug: - The page doesn't scroll at all This will be fixed in the forward port. This commit simply manually closes the collapse before starting our own smooth scrolling. It also targets the desktop offcanvas menu (when hamburger layout is selected). opw-3604963
qsm-odoo
force-pushed
the
17.0-fix-offcanvas-mobile-anchor-rde
branch
from
d511d91 to
9633a7e
Compare
qsm-odoo
approved these changes
Dec 19, 2023
There was a problem hiding this comment.
- Made this change
- Fixed a small typo in commit message "worst" instead of "worse"
@robodoo r+
robodoo
pushed a commit
that referenced
this pull request
Dec 19, 2023
A anchor menu in the mobile collapse related to an element in the current page doesn't work correctly: - The collapse-menu doesn't close - If the menu is large enough, on mobile, you actually don't even see the page scrolling behind it so it looks like the click did nothing. This is probably because of the switch from BS4 to BS5. In later version, it's even worse, as we are using bootstrap Offcanvas instead of collapse, creating another bug: - The page doesn't scroll at all This will be fixed in the forward port. This commit simply manually closes the collapse before starting our own smooth scrolling. It also targets the desktop offcanvas menu (when hamburger layout is selected). opw-3604963 closes #146132 Signed-off-by: Quentin Smetz (qsm) <[email protected]>
This was referenced Dec 19, 2023
3 similar comments
luanjubica
pushed a commit
to luanjubica/odoo-code
that referenced
this pull request
Feb 14, 2024
A anchor menu in the mobile collapse related to an element in the current page doesn't work correctly: - The collapse-menu doesn't close - If the menu is large enough, on mobile, you actually don't even see the page scrolling behind it so it looks like the click did nothing. This is probably because of the switch from BS4 to BS5. In later version, it's even worse, as we are using bootstrap Offcanvas instead of collapse, creating another bug: - The page doesn't scroll at all This will be fixed in the forward port. This commit simply manually closes the collapse before starting our own smooth scrolling. It also targets the desktop offcanvas menu (when hamburger layout is selected). opw-3604963 closes odoo#146132 Signed-off-by: Quentin Smetz (qsm) <[email protected]>
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.
A anchor menu in the mobile offcanvas related to an element in the
current page doesn't work in offcanvas.
This is because the menu anchor navigation is hooked to use our own
scrolling behavior instead of the browser one.
Doing so, we preventDefault, which prevent the offcanvas menu to close
itself when clicking on a anchor menu.
This commit simply manually close the offcanvas and once the closing
animation is complete, starts our own smooth scrolling.
Another possibility would have been to just close manually the offcanvas
without a preventDefault and without a call to our custom scrolling
method.
Doing so, the browser would naturally scroll to the element while we
close the offcanvas but it would be less elegant as you wouldn't see the
scrolling animation.
Note that the offcanvas was introduced with commit 1.
opw-3604963