Navigation Overlay: Improve handling of open overlay. #32886
Merged
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
|
Size Change: +557 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
jasmussen
force-pushed
the
fix/responsive-preview
branch
2 times, most recently
from
9adf49a to
f34022a
Compare
|
In testing, the block toolbar is unattached and floating off to the right
|
|
Thank you for testing. That's curious, I can't directly reproduce. Here's trunk:
Here's this branch:
Clearly there's still some work to be done (though arguably an entirely missing block toolbar might be better than an arbitrarily floating one?). Oh, it just hits me you're using the site editor, where the block toolbar is outside the iframe and therefore in a different stacking order where it can overlay. Alright, so what it seems like is that the block toolbar is actually attached to the parent menu item, the one outside of the overlay menu. The menu you see inside the overlay is actually a duplicate. It seems like we might need to address this separately, CC: @vcanales. Based on the headaches we've encountered with this PR, the question is whether overlay menu editing needs to happen in its own instance entirely. See #29148 for a conversation on isolated editing. In the mean time, since the situation in trunk is not ideal, I wonder if we should land this one as an interim bandaid? |
jasmussen
force-pushed
the
fix/responsive-preview
branch
from
f34022a to
31fd743
Compare
|
I pushed a fix for the iframe, and can see the block toolbar there. I also figured out that unlike what I said before, the contents of the overlay is actually the navigation block, but the positioning is okay for me. |
mkaz
approved these changes
Jul 1, 2021
12 tasks
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.
Description
The navigation overlay menu, the one you get when you enable "Responsive menu" on the navigation block, can be opened inside the editor. When that happens, you see the full overlay just like you do on the frontend.
However where
position: fixed;for the fullscreen menu is fine on the frontend because it literally needs to cover everything, when inside the editor, we still need to see the UI, like the top toolbar, the sidebar, etc. And we can't useposition: absolute;because there are some ancestors settingposition: relative;, breaking that.This PR adds:
is-mobile-previewandis-tablet-previewandis-desktop-preview.I suspect the device preview classes are useful in other contexts, but let me know if you'd prefer me to find another way to handle this.
A silver lining is: all of this positioning stuff can be removed when this lands in an iframe, including making the preview accept the device preview sizings.
How has this been tested?
Insert a navigation menu with some menu items. Enable responsive menu. Add a background color so you can more easily see the overlay. Now scale the viewport small so the burger menu becomes visible. Then click the burger menu and resize back.
Now try combinations of device preview, top toolbar, fullscreen, and breakpoints.
Checklist:
*.native.jsfiles for terms that need renaming or removal).