Refactor Nav Block Overlay CSS #56037
Closed
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
getdave
added
[Type] Code Quality
|
Size Change: -864 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
scruffian
reviewed
Nov 10, 2023
| // Inherit as much as we can regarding colors, fonts, sizes, | ||
| // but otherwise provide a baseline. | ||
| // In a future version, we can explore more customizability. | ||
| .wp-block-navigation__responsive-container.is-menu-open { |
There was a problem hiding this comment.
Why don't we move this to the other file too?
There was a problem hiding this comment.
Because I believe all the styles and sub selectors within this are not nav-block agnostic. They are specifically related to styling the Navigation block.
I want the other file to be just "overlay" stuff specifically.
Is there a better option? Do you think I should move it and if so why?
There was a problem hiding this comment.
If we want to make an agnostic component, Dave's idea is correct, but we do want to separate things that are specific to the navigation if they pertain to the wider viewport or to the collapsed version of it. Where that should live is another matter.
There was a problem hiding this comment.
I was thinking about this some more. This type of overlay may well be specific to the Navigation block. I still think it's helpful to extract the overlay styles from other behaviour that handles sub blocks and things.
This code is unlikely to end up being a standalone <Overlay> component because it's heavily tied to a specific responsive behaviour which shows and hides specific pieces of the overlay depending on an arbitrary breakpoint. That is unlikely to be portable to other scenarios.
There was a problem hiding this comment.
I think you might be right. We can refactor again if we feel like we need to, and it will already be way cleaner than it was anyway
|
Let's keep an eye on the e2e tests, they might be relevant to the changes |
|
|
||
| // ******************************************************** | ||
| // TOGGLE BUTTONS | ||
| // Alias: |
There was a problem hiding this comment.
I like the agnostic classes. We could use @extend to keep backwards compatibility with the old classes
getdave
commented
Nov 10, 2023
Comment on lines
+41
to
+49
| @include nav-breakpoint { | ||
| &:not(.hidden-by-default):not(.is-overlay-open) { | ||
| display: block; | ||
| width: 100%; | ||
| position: relative; | ||
| z-index: auto; | ||
| background-color: inherit; | ||
| } | ||
| } |
There was a problem hiding this comment.
I assume this code exists so that if the overlay is open and the screen size changes then the overlay becomes hidden?
I'm wondering. If in the future we want to allow the Nav breakpoint to change dynamically based on measuring the width of the Navigation block, then we might like to look at moving all the responsive behaviour to JavaScript with something like matchMedia similar to what I did in #45274.
The only issue is then we'd need to find a way to do a similar thing with directives on the front of the site.
I believe taking such an approach would make this code a lot simpler.
There was a problem hiding this comment.
yes, I was thinking on using a class that will be added or removed once we reach the correct viewport width
|
Do you feel like we should keep working gon this? I can make time to have a look at the tests and check if we are breaking something with the changes |
Probably not a priority. I'll close it out. |
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?
WIP
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast