Navigation: Refactor mobile overlay breakpoints to JS #57520
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
|
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) > & { |
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
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' : '', |
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
Comment on lines
547
to
548
| $nav_element_directives .= 'data-wp-init="callbacks.initNav"'; | ||
| $nav_element_directives .= 'data-wp-class--is-collapsed="context.isCollapsed"'; |
There was a problem hiding this comment.
Won't these end up joined without spaces between the two directives?
| @@ -37,3 +37,5 @@ export const SELECT_NAVIGATION_MENUS_ARGS = [ | |||
| 'wp_navigation', | |||
| PRELOADED_NAVIGATION_MENUS_QUERY, | |||
| ]; | |||
|
|
|||
| export const NAVIGATION_MOBILE_BREAKPOINT = 600; | |||
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 <[email protected]>
Co-authored-by: Dave Smith <[email protected]>
|
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
lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php
Outdated
Show resolved
Hide resolved
getdave
approved these changes
Jan 4, 2024
Co-authored-by: Dave Smith <[email protected]>
I asked @luisherranz for a review off GitHub and he was happy with the approach :) |
scruffian
added
the
[Type] Code Quality
|
✅ 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. |
|
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 <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: DAreRodz <[email protected]> Co-authored-by: huubl <[email protected]> Co-authored-by: cbirdsong <[email protected]> Co-authored-by: tomxygen <[email protected]>
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
…" (#59149) Co-authored-by: c4rl0sbr4v0 <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: DAreRodz <[email protected]> Co-authored-by: huubl <[email protected]> Co-authored-by: cbirdsong <[email protected]> Co-authored-by: tomxygen <[email protected]>
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
…" (#59149) Co-authored-by: c4rl0sbr4v0 <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: DAreRodz <[email protected]> Co-authored-by: huubl <[email protected]> Co-authored-by: cbirdsong <[email protected]> Co-authored-by: tomxygen <[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.
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