Conversation
61fbfd6 to
50429bf
Compare
|
While I love the idea of adding multiple buttons to a block, I keep thinking, should this just be part of the Button block? For example, with the addition of the Social block, I don't have a Social Block AND a Socials block. Whether it's one or many, it's all in one block. This, for me, seems how the Button block should work. It appears there wasn't a strong preference in the issue #16480, and I'm completely open to being convinced otherwise, but I feel pretty strongly toward including this in the existing Button block instead of creating another block. |
|
It seems really weird and confusing to have a button block and a buttons block. It would be better to improve the existing button block and also rename it to buttons. |
|
|
50429bf to
578c1f2
Compare
Hi @mtias, this PR was updated and now follows this logic. Only buttons block is available, button can still be used in existing content and existing templates. |
578c1f2 to
ca64d9a
Compare
|
I tested this and it looks great 👏 |
|
Is this being backported to WP 5.3? |
No, for next release. Too many changes already. |
|
|
||
|
|
||
| .wp-block-buttons { | ||
| // 1. Reset margins on immediate innerblocks container. |
There was a problem hiding this comment.
Can we extract all of this to InnerBlocks horizontalDisplay?
There was a problem hiding this comment.
I extracted this CSS logic into generic InnerBlocks flags.
b279dda to
90c1f46
Compare
c398b47 to
e364932
Compare
| if ( setting === 'opensInNewTab' ) { | ||
| onToggleOpenInNewTab( value ); | ||
| } | ||
| } } |
There was a problem hiding this comment.
The API is a bit weird to me. That's feedback that is not meant directly to this PR but more about the LinkControl component. cc @getdave
I think ideally, the current link prop is just another setting and we should have a single onChange handler. Any particulra reason to separate both?
I also feel we should separate the definition of the settings (id, title) from the value (checked).
Should we also absorb handleLinkControlOnKeyDown inside the component itself?
… part of InnerBlocks), removed toolbar movers (they are now part of InnerBlocks)
b9afda0 to
9b44f75
Compare
|
I've been seeing quite a few intermittent build failures recently which appear to stem from the test file introduced in this pull request. Example: https://travis-ci.com/WordPress/gutenberg/jobs/272683577 |
|
The alignment buttons don't work. See #19616. |
| const handleLinkControlOnKeyPress = ( event ) => { | ||
| event.stopPropagation(); | ||
| }; |
There was a problem hiding this comment.
What purpose does this serve? How would a future maintainer be expected to interpret this requirement to avoid a regression?
A few suggestions:
Event#stopPropagationis a code smell, and we should avoid it as much as possible- When we have no alternative, we should document exhaustively why we're stopping propagation. Something like an inline comment or, in this case, even just naming the function in such a way which communicates the intention.
|
@aduth do you know if this is going to be added to the core gutenberg anytime soon? |
|
@cinghaman This is already available in the latest version of the plugin. See: https://make.wordpress.org/core/2020/01/09/whats-new-in-gutenberg-8-january/ Based on the regular development schedule, it would be expected this should be available in the next major release of WordPress (5.4). |
Description
Closes: #16480
This PR adds a buttons block. A container for buttons. It is starting point right now it is not much more than a container but we can follow up from this and discussi new features to add.
How has this been tested?
I tested the buttons and button block work as expected by doing some smoke tests.
Screenshots