Navigation: Add an option to allow navigation to switch to overlay mode when wrapping #57587
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
scruffian
added
[Type] Enhancement
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/editor-settings.php ❔ lib/experiments-page.php |
5 tasks
|
Size Change: +1.92 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
getdave
reviewed
Jan 5, 2024
There was a problem hiding this comment.
Performance wise the flickering is probably resulting from layout thrashing.
Below is a good resource for working out which properties read/write will cause this.
getdave
reviewed
Jan 5, 2024
getdave
reviewed
Jan 5, 2024
getdave
reviewed
Jan 5, 2024
scruffian
commented
Jan 5, 2024
scruffian
commented
Jan 5, 2024
scruffian
commented
Jan 5, 2024
scruffian
commented
Jan 5, 2024
| import { NAVIGATION_MOBILE_COLLAPSE } from './constants'; | ||
| import navigationIsWrapping from './is-wrapping'; | ||
|
|
||
| function useIsCollapsed( overlayMenu, navRef ) { |
There was a problem hiding this comment.
Something about this hook is buggy. Its possible to see a wrapped navigation block in the editor.
scruffian
requested review from
spacedmonkey and
tellthemachines
as code owners
This comment was marked as outdated.
This comment was marked as outdated.
Comment on lines
593
to
596
| <ToggleGroupControlOption | ||
| value="auto" | ||
| label={ __( 'Auto' ) } | ||
| /> |
There was a problem hiding this comment.
Should we check if the nav block is set to show horizontal/vertical before showing this option? It doesn't make much sense if it's vertical
|
There is a case that has not been considered: when the navigation is in a flex container but it's the first element inside it. If it's big enough, it will make whatever siblings are in the wrapper move underneath it and it will only trigger the overlay when there is no more space for the nav items: I think that makes sense and don't think we should consider (it would be too much complexity) but I don't know if RTL languages will use this pattern often or not. In any case, I think it shouldn't be considered a bug of this implementation. |
"Auto" seems reasonable to me, "Overflow" might be another option. We may need to switch to a dropdown regardless accounting for i18n. This would present an option to include a description for each value which might help too? |
45 tasks
I think a description would be really helpful |
|
I updated the design to use a 
|
scruffian
commented
Jan 11, 2024
| } | ||
| window.addEventListener( 'resize', debounce( updateIsCollapsed, 100 ) ); | ||
| return () => { | ||
| window.removeEventListener( 'resize', updateIsCollapsed ); |
There was a problem hiding this comment.
Given that the function attached to the resize event is debounced, I'm not sure this will work.
There was a problem hiding this comment.
Correct. You'll have to define a variable that refers to the debounced function and then provide that variable to the removeEventListener. Debounce returns a new function reference.
|
This would be a nice addition to the block if it can detect the context accurately, which I understand is the main concern. From my experience dealing with low-code end-users, block structures can get wild (unintentionally, in most cases) with Groups within Groups within Columns (sometimes a single column) and so forth. If this option can't parse through that and understand which width constraints are "artificial" and which are real, I feel like this could be a source of confusion. Regarding the ways the option is shown, I much prefer the select option because it's more descriptive (also "auto" would be hard to distinguish from "mobile"), but I think "Navigation Overlay" doesn't feel describe what that option does. I wonder if we could go with a more common terminology, like "Navigation Responsiveness", "Responsive Navigation", "Mobile Navigation", "Mobile Behavior" or similar. |
The word mobile doesn't really work here, because only one of the options relies on the width of the viewport to trigger the burger menu (Mobile). Never will never trigger it, Auto depends on content, not viewport size and Always will show the overlay on "desktop" too. Wording is really hard, but I think responsive/mobile descriptors here are misleading. |
MaggieCabrera
force-pushed
the
add/auto-wrapping
branch
from
ee2afbf to
67e700f
Compare
draganescu
reviewed
Jan 29, 2024
There was a problem hiding this comment.
This is a great approach to the problem. It does indeed not solve setting a breakpoint, but it does alleviate the actual need for that in many situations, in a clever way 👏🏻 I think that:
- it should be, like Joen suggested, the default behavior so that in the future we may improve on the implementation (so we should add no new UI for this)
- it can be the default behavior if we make sure we don't have too many obvious edge cases
- maybe, for the really edge edge cases people can select the other modes for collapsing behavior
What I don't like, personally, in this PR is that we also refactored other code so when we introduce the experiment we may leak some other changes. It would be great if we could not do the code shuffling even if that leads to duplication for the experiment.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
...but only for newly inserted Nav blocks. If you have already got a Nav block and you left the default settings, I don't think we should be amending those defaults. Otherwise we're going to see a number of sites break if/when this becomes a non-expertiment. |
|
@scruffian I think this is not the right way to go, especially thinking that the grid block is getting more work on and that will definitely make this solution way more complicated than it is. I don't want to close your PR even though we both worked on it |
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.
What?
Addresses #45040
https://codepen.io/Marga-Cabrera/pen/WNPjYYw
This PR adds a new option for the nav block overlay called "auto". When this option is enabled a script will detect if the nav block can fit within its own container. If it detects that it's wrapping, it will toggle the overlay burger automatically, instead of using the fixed breakpoint.
We do two checks here:
flex-wrap. Based on that assumption I look for said parent recursively and apply the same principle to verify if the children inside it are wrapping or not. In my personal opinion, if we are not covering this case, there is very little use in continuing this path, since 90% of the time, this is what's causing the nav block to wrap ahead of time and not the amount of items inside it.To do:
Why?
After the discussion over at #54388 and some first explorations, it becomes really clear that we either allow our users to set a breakpoint manually or we need to use JavaScript to detect when. Given the complexity of the nav block and how many combinations of layouts it can be wrapped in, there is no way CSS can only give a solution that will work for most of the use cases.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
This adds a new option to the toggle:
I don't think this is clear enough on its own. Maybe it should be called wrap? Maybe we should use a dropdown instead of the toggle group? cc @WordPress/gutenberg-design