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

Layout: Show vertical alignment toolbar with allowSwitching enabled #67022

Merged
merged 1 commit into from
Nov 18, 2024
+0 −3
Merged

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any further context but agree this check isn't the right one here.

One thing I'm noticing now is that setting allowJustification to false doesn't actually disable the justification controls for flex layout either in the toolbar or the sidebar. That should also be addressed, but not necessarily as part of this PR.

Given that this change by itself fixes #67016 and doesn't affect anything else, I'm ok with merging this as is and fixing the justification controls separately.

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