Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Block Library - Navigation]: Fix vertical layout #37009

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Nov 30, 2021

Fixes: #36902

This PR fixes a regression introduced here: #36292.

Testing instructions

  1. In a page/post add a Navigation block and add a couple of items
  2. Change the layout orientation to vertical (do not change content justification)
  3. Save
  4. View the frontend and observe that the Navigation is vertical now

In general I think something like this should be used to get the default block layout in edit function. Now in Navigation block we provide some layout defaults which seems that it shouldn't - I haven't checked in depth yet as the block itself is really convoluted.

The specific problem was that we needed the --layout-direction attribute which wasn't added if layout.justifyContent was empty.

The logic about setCascadingProperties needs more looking. I noticed the 'spread' of items in vertical orientation (Search in Navigation, Social Links) and I believe there must be similar css rules like this fix.

Screen.Recording.2021-11-30.at.8.39.43.PM.mov

@ntsekouras ntsekouras added [Type] Regression Related to a regression in the latest release [Block] Navigation Affects the Navigation Block Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Nov 30, 2021
@ntsekouras ntsekouras self-assigned this Nov 30, 2021
@jasmussen
Copy link
Contributor

Thanks for the PR. What's a good way to test it?

@ntsekouras
Copy link
Contributor Author

Thanks for the PR. What's a good way to test it?

Sorry about that 😄 - I updated the PR's description

@ntsekouras ntsekouras force-pushed the fix/navigation-block-vertical-layout branch from 20fbbd3 to c8d14bd Compare December 1, 2021 08:54
@jasmussen
Copy link
Contributor

Thank you. Tests well for me. I can't speak well to the inner workings, but I'll be happy to give a thumbs up if you have high confidence in this one.

@ntsekouras
Copy link
Contributor Author

Thank you. Tests well for me. I can't speak well to the inner workings, but I'll be happy to give a thumbs up if you have high confidence in this one.

In general we need to review the whole setCascadingProperties code as it seems to have coupled the layout abstraction with specific block needs. Until then though, this is needed, yes.

@ntsekouras ntsekouras merged commit 84cbc23 into trunk Dec 3, 2021
@ntsekouras ntsekouras deleted the fix/navigation-block-vertical-layout branch December 3, 2021 13:08
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 3, 2021
@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation block: vertical orientation not working
3 participants