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.

Sign up for GitHub

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

Jump to bottom

Layout: Show vertical alignment toolbar with allowSwitching enabled #67022

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Layout: Show vertical alignment toolbar with allowSwitching enabled #67022

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/block-editor/src/layouts/flex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,6 @@ export default {
onChange,
layoutBlockSupport,
} ) {
if ( layoutBlockSupport?.allowSwitching ) {
return null;
}
Comment on lines -97 to -99
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this early return condition needs to be fixed. It should probably return null when both allowJustification and allowVerticalAlignment are false.

@ntsekouras, can you confirm? I think you worked on the initial implementation of this feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been so long and even by looking at old PRs, I'm still not 100% sure why this check was added. If I remember correctly at that time we explicitly didn't want to show the justification controls in inspector controls too. Having said that, back then there were fewer layout props and there was no vertical alignment.

The allowJustification prop was added specifically for constraint layout but in docs says that can also be applied to flex and defaults to true. There are no checks for it now in flex layout, so I guess we need to take into account both in inspectorControls and toolBarControls.

So, I'm not sure whether the allowSwitching check is still needed, but the other checks (allowVerticalAlignment, allowJustification) should be added.

I'll cc @tellthemachines if she has more context.

const { allowVerticalAlignment = true } = layoutBlockSupport;
return (
<BlockControls group="block" __experimentalShareWithChildBlocks>
Expand Down
Loading