Navigation: Refactor mobile overlay breakpoints to JS#57520
Merged
Navigation: Refactor mobile overlay breakpoints to JS#57520
Conversation
|
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/compat/wordpress-6.5/class-wp-navigation-block-renderer.php |
scruffian
commented
Jan 3, 2024
| @include break-small { | ||
| display: none; | ||
| } | ||
| :not(.is-collapsed) > & { |
Contributor
Author
There was a problem hiding this comment.
The > selector is necessary because the class gets added to the nav element, but it does make the CSS a bit more complex.
scruffian
commented
Jan 3, 2024
|
Size Change: +675 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
scruffian
commented
Jan 3, 2024
scruffian
requested review from
spacedmonkey and
tellthemachines
as code owners
getdave
reviewed
Jan 4, 2024
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
I think the concept is good here. We need to optimise the code and I've left suggestions on how we could do this 🤞
Comment on lines
-400
to
-409
| $is_hidden_by_default = isset( $attributes['overlayMenu'] ) && 'always' === $attributes['overlayMenu']; | ||
|
|
||
| $responsive_container_classes = array( | ||
| 'wp-block-navigation__responsive-container', | ||
| $is_hidden_by_default ? 'hidden-by-default' : '', | ||
| implode( ' ', $colors['overlay_css_classes'] ), | ||
| ); | ||
| $open_button_classes = array( | ||
| 'wp-block-navigation__responsive-container-open', | ||
| $is_hidden_by_default ? 'always-shown' : '', |
Contributor
There was a problem hiding this comment.
Given that this code is super confusing I'll be glad to see the back of it.
getdave
reviewed
Jan 4, 2024
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Looking much better now.
On testing I found that everything worked in the editor, but on the front of the site the Nav block set to Always simply behaved as per the one set to Mobile.
Screen.Capture.on.2024-01-04.at.14-27-49.mp4
Comment on lines
+547
to
+548
| $nav_element_directives .= 'data-wp-init="callbacks.initNav"'; | ||
| $nav_element_directives .= 'data-wp-class--is-collapsed="context.isCollapsed"'; |
Contributor
There was a problem hiding this comment.
Won't these end up joined without spaces between the two directives?
| PRELOADED_NAVIGATION_MENUS_QUERY, | ||
| ]; | ||
|
|
||
| export const NAVIGATION_MOBILE_BREAKPOINT = 600; |
Contributor
There was a problem hiding this comment.
Should we try and establish a consistent terminology here of "collapsed" as you have elsewhere?
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Contributor
Author
|
Thanks for the review @getdave. I think this is ready for another review. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
getdave
reviewed
Jan 4, 2024
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Based on my testing this now seems to match up with behaviour on trunk which is the desired outcome.
Left a few suggestions and also suggested getting more 👀 on the directives stuff as it's not my area of expertise.
lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php
Outdated
Show resolved
Hide resolved
getdave
approved these changes
Jan 4, 2024
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Approving based on #57520 (review)
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Contributor
Author
I asked @luisherranz for a review off GitHub and he was happy with the approach :) |
scruffian
added
the
[Type] Code Quality
Contributor
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR no longer requires a backport for WP 6.5. Why? Because the code was moved into the block-library package in #57979 and thus should be automatically handled by the packages sync process. cc @scruffian for confirmation. |
Contributor
Author
|
Confirming that is also my understanding :) |
cbravobernal
added a commit
that referenced
this pull request
Feb 19, 2024
This reverts commit 1b75cf6.
fabiankaegy
added a commit
that referenced
this pull request
Feb 19, 2024
…" (#59149) Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: huubl <huubl@git.wordpress.org> Co-authored-by: cbirdsong <cbirdsong@git.wordpress.org> Co-authored-by: tomxygen <tomxygen@git.wordpress.org>
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
…" (#59149) Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: huubl <huubl@git.wordpress.org> Co-authored-by: cbirdsong <cbirdsong@git.wordpress.org> Co-authored-by: tomxygen <tomxygen@git.wordpress.org>
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
…" (#59149) Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: huubl <huubl@git.wordpress.org> Co-authored-by: cbirdsong <cbirdsong@git.wordpress.org> Co-authored-by: tomxygen <tomxygen@git.wordpress.org>
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
3 participants
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?
This moves the logic for collapsing the navigation block to its overlay mode into Javascript.
Why?
This is needed as part of the work in #56781. I'm doing it in a different PR to keep that other PR smaller.
How?
Created a new class called 'is-collapsed' which handles the state of the whole block.
Testing Instructions