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

Allow Spacer block to inherit orientation from layout #36197

Closed
tellthemachines opened this issue Nov 4, 2021 · 5 comments · Fixed by #49322
Closed

Allow Spacer block to inherit orientation from layout #36197

tellthemachines opened this issue Nov 4, 2021 · 5 comments · Fixed by #49322
Assignees
Labels
[Block] Spacer Affects the Spacer Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@tellthemachines
Copy link
Contributor

Description

Currently, the Spacer block has the wrong orientation when inside a Row block, because Spacer reads orientation from block context, but Row has orientation defined as part of its layout attribute.

If #36169 goes through, we'll have the same problem when Spacer is a child of Navigation.

While we could try a patch along the lines of

const { orientation } = layout;

	useEffect( () => {
		if ( orientation ) {
			setAttributes( { orientation } );
		}
	}, [ orientation ] );

in the parent block's edit function, we'd have to add it to all possible parents of Spacer with configurable orientation, and it could break if anything changes in the layout logic. It would also require a default value, because orientation isn't set explicitly inside layout unless it's changed by the user.

Spacer needs the orientation attribute not for styling purposes, but so it can set the direction of its controls, so merely inheriting its parent's layout isn't much use. Ideally we'd be able to provide layout properties as block context, so that Spacer could still read the attribute.

Other ideas welcome!

Step-by-step reproduction instructions

  1. Add a Row block
  2. Inside it, add a Spacer block
  3. Verify that Spacer controls are still vertical.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@talldan
Copy link
Contributor

talldan commented Feb 7, 2022

I closed #30590, which duplicated this issue.

The current situation seems to be that #36340 fixed the spacer orientation in the navigation block.

The bug still exists for the Row variation of the Group block—the spacer is in the wrong orientation still. It can be used to make the row taller, but it won't space items.

@andrewserong
Copy link
Contributor

The bug still exists for the Row variation of the Group block—the spacer is in the wrong orientation still.

I've come up with a potential fix for this in #39743, which passes the layout attribute down from the Group block to the Spacer block via context. The Spacer block then attempts to get orientation or layout?.orientation from the context. If it cannot find that, then if the layout?.type is set to flex it infers horizontal orientation. It seems to be working pretty well, but it does mean that the Spacer block needs to "know" a bit more about how layouts work, so it does introduce a little bit of coupling there 🤔

@andrewserong andrewserong self-assigned this Mar 27, 2022
@tellthemachines
Copy link
Contributor Author

@andrewserong initially when I wrote this out I was thinking of setting up layout to always be present as context, instead of having to add it as context manually to each block.

If we were to hack the existing code, that would require adding block supports info to the block representation in the store, and possibly providing context for every block by default. I'm not sure if we want to do those things though.

Or maybe we could try creating a separate layout context provider?

I opened this more to get a discussion going and ideas on if/how this might be possible. If it turns out it's not possible or we don't want to do it, it's probably easier to just manually add the orientation as an attribute in the context provider block, like we do in Navigation.

@tellthemachines
Copy link
Contributor Author

Another case where the Spacer block needs to know about parent layout is #38022.

@andrewserong
Copy link
Contributor

Thanks for the extra context @tellthemachines! I stumbled upon this issue the backwards way by first having a go at the PR #39743 and then checking to see if there was already an open issue for it 😄

I'm AFK for the rest of this week, but will take a deeper look at the feedback on that PR and in that other linked issue (#38022) to get a better idea of the broader context surrounding passing down layout context once I'm back.

In the meantime, if anyone's keen to have a play with it, feel free to take over that PR! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Spacer Affects the Spacer Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
3 participants