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

Remove: Alignment options from button nested inside buttons #19824

Merged

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Jan 22, 2020

Fix: #19616

When a button block is inside a buttons block, we should not render alignment options for the button block. The alignment is controlled globally for all blocks on the buttons block.

This change is complex because we can not simply remove the alignment from the button block, as for existing "button" blocks outside buttons the alignment on the block should still be supported.

If the alignment toolbar was not being rendered in a popover hiding it with CSS would be an option but given the change to popover that is not an option.
I thought of adding another filter addFilter( 'editor.BlockEdit', 'core/editor/align/with-toolbar-controls', withButtonBlockAlignRemover );. The filter would remove the added alignment toolbar for button blocks whose parent is a buttons block. But, doing that would mean we would add a dependency on WordPress hooks in our block library.

So it seems with the current API's it was not possible to hide the alignment toolbar of a button block when nested inside buttons block.
I exposed an API component AlignmentHookSettingsProvider in block editor that allows parent blocks to provide settings for the alignment hook. The current setting we support is getContextualValidAlignments that allows changing what alignments are supported based on the expected valid alignments for a given block and the props a block receives.

I prefer a change that does not involve additions to the public API, but it seems it is something not possible. I'm open to other ideas.

How has this been tested?

I added a buttons block and some "button" blocks inside.
I verified it was not possible to change the alignment of a "button" block.
I verified I could change the alignment of a button block.
I pasted this code in the block editor to create a top-level button:

<!-- wp:button -->
<div class="wp-block-button"><a class="wp-block-button__link">Top Level Button</a></div>
<!-- /wp:button -->

I verified I could change the aligment of that button block.

@@ -71,6 +71,10 @@ registerCoreBlocks();

<!-- START TOKEN(Autogenerated API docs) -->

<a name="AlignmentHookSettingsProvider" href="#AlignmentHookSettingsProvider">#</a> **AlignmentHookSettingsProvider**

Undocumented declaration.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jan 22, 2020

Author Member

This will be properly documented if we are fine with this approach. But I would prefer to have some feedback on the general mechanism before documenting and polishing it.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 23, 2020

Did you consider migrating existing button blocks to buttons blocks. I know we don't have a way for deprecations to change the block name but we discussed several times whether we should add it.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Jan 23, 2020

Did you consider migrating existing button blocks to buttons blocks. I know we don't have a way for deprecations to change the block name but we discussed several times whether we should add it.

Hi @youknowriad, In this case, it is complex, because the button block still exists and is valid but now is inside the buttons block. We would need an API not only to migrate a block to another one but also to consider block invalid if it is not nested inside a specific block.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 23, 2020

Mmm right, let's avoid it then.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 24, 2020

So a button block outside the buttons blocks is possible? Why not force it to be in a buttons block?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 24, 2020

So a button block outside the buttons blocks is possible? Why not force it to be in a buttons block?

It's not possible today but some existing posts already have it.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Jan 24, 2020

The whole Button block is a bit odd; technically, it's not even an actual button. It's a link; it's an <a> element.

I do think there is a need for parent blocks to be capable of controlling what alignments are considered valid inside them. For example, a flexbox container can't have floated children (alignleft, alignright), so any blocks that use flexbox styling would want to disable float alignments for their immediate children. (Note that this should not propagate to children of children.)

@jorgefilipecosta jorgefilipecosta force-pushed the remove/alignment-from-button-nested-inside-buttons branch from e71b735 to 4567d94 Jan 29, 2020
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Jan 29, 2020

Hi Zebulan, thank you for sharing your thoughts. I guess the button block was named because button is more user-friendly but for accessibility reasons, we should use the "a" element.

so a button block outside the buttons blocks is possible? Why not force it to be in a buttons block?

It's not possible today but some existing posts already have it.

Exactly.

Hi @youknowriad, @ellatrix any additional thoughts regarding this solution? Unfortunately, we can not use make sure the alignment we are setting will only pass one level down, as react context passes multiples levels down. I guess an alternative would be to make alignment restrictions part of InnerBlocks but that option is more complex.
Also given that we are not totally aware of the best path here would it make sense to merge an experimental solution in this case (currently it is not experimental).

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 30, 2020

I believe the context can be simplified (instead of providing supported alignments, it could just tell that it's an embedded button). that said, no strong opinions against the solution, we should just make sure it's private API.

@jorgefilipecosta jorgefilipecosta added this to Needs Review in WordPress 5.4 Must Have Feb 6, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the remove/alignment-from-button-nested-inside-buttons branch from 4567d94 to 6f51e5e Feb 7, 2020
…to control alignment options from ancestors;
@jorgefilipecosta jorgefilipecosta force-pushed the remove/alignment-from-button-nested-inside-buttons branch from 6f51e5e to 8da1289 Feb 7, 2020
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Feb 7, 2020

Hi @youknowriad, unfortunately, we don't have a way to communicate from a block to a hook in block editor privately. I made the API used as experimental and followed your suggestion to simplify and juts specify if we are in an embed button context.
Would you be able to provide a final review? Thank you in advance!

@@ -1,7 +1,10 @@
/**
* WordPress dependencies
*/
import { InnerBlocks } from '@wordpress/block-editor';
import {
__experimentalAlignmentHookSettingsProvider as AlignmentHookSettingsProvider,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 7, 2020

Contributor

I guess my thinking was that it's the responsibility of the "buttons" to create the context and provide it while the "button" would consume it. And this is possible with the private API.

It seems the fact that we use the "align support" make this impossible.

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Feb 7, 2020

Contributor

@youknowriad Maybe we should implement a hook for alignment, similar to the useColors hook, and start using that everywhere instead of the supports flag. Would that make what you want to do possible?

@jorgefilipecosta jorgefilipecosta merged commit a8f6bda into master Feb 7, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Feb 7, 2020
@jorgefilipecosta jorgefilipecosta deleted the remove/alignment-from-button-nested-inside-buttons branch Feb 7, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.