Skip to content
This repository was archived by the owner on Oct 28, 2022. It is now read-only.

Conversation

mikachan
Copy link
Member

@mikachan mikachan commented Aug 24, 2022

This PR is to help test out WordPress/gutenberg#43576

I've updated this PR so that it should be in a good state to bring in, now that the above GB PR has been merged.

The changes include:

  • adjusts root vertical padding so that it's consistent (top and bottom now spacing-40)
  • removes the top padding from the header and bottom padding from the footer, as they're no longer needed with the root padding change
  • changes the spacing variables in the JSON markup (part of Spacing: replace var() format with var:preset|spacing|value #95)

@jasmussen
Copy link

How is it feeling? You should be able to have different padding values for all 4 sides and so long as there aren't any additional compounding paddings, it should line up.

The biggest questionmark, I think, is the max-width. Right now it defaults to the content width because it felt the most sensible for the header. But it could default to wide or full as well. 🤔

@mikachan
Copy link
Member Author

Sorry, I ran out of time yesterday to write down any notes after testing this!

It seems to work great with TT3 so far on a smaller resolution, or where the content fills the available screen width. I needed to add a top root padding value, as TT3's was set to 0px (for no specific reason), otherwise, the close button had no top padding (correctly, I believe).

Screen.Recording.2022-08-25.at.09.40.10.mov

Currently, the max-width doesn't work well for TT3 at larger resolutions, as the header is set to wide align:

Screen.Recording.2022-08-25.at.09.46.40.mov

So in this case, it would be better if it defaulted to --wp--style--global--wide-size. It's a shame it can't easily use the available space, I guess this is because of the position: fixed on the overlay.

@jasmussen
Copy link

Good thoughts. I'll update the value in the PR to --wp--style--global--wide-size, as that does use more of the available space and seems to cater to more common designs.

In the future, I like to think we can improve this code further, so it more naturally stays in sync with the content width of the header part.

@jasmussen
Copy link

Changed to wide width.

@mikachan
Copy link
Member Author

Changed to wide width.

This works great!

Screen.Recording.2022-08-25.at.10.34.16.mov

@jasmussen
Copy link

Cool! Let's test the core PR and see that nothing regresses in other themes, then we can land it! 🥳

@mikachan mikachan changed the title Test Navigation Toggle GB PR Improve placement of navigation overlay close button Aug 27, 2022
@mikachan mikachan added [Type] Enhancement New feature or request and removed [Type] Gutenberg labels Aug 27, 2022
@mikachan mikachan marked this pull request as ready for review August 27, 2022 09:40
@mikachan mikachan changed the title Improve placement of navigation overlay close button Update header and footer spacing Aug 27, 2022
@mikachan mikachan merged commit fc6e4a2 into trunk Aug 27, 2022
@mikachan mikachan deleted the test/navigation-toggle branch August 27, 2022 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
[Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants