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

Add: Buttons block #17352

Merged
merged 5 commits into from Jan 3, 2020
Merged

Add: Buttons block #17352

merged 5 commits into from Jan 3, 2020

Conversation

jorgefilipecosta
Copy link
Member

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

Sep-05-2019 22-23-55

@mapk
Copy link
Contributor

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.

@jorgefilipecosta
Copy link
Member Author

Hi @mapk, technically it is possible to follow this path and have a button block that can contain multiple buttons I don't see any blocker to this.
In the issue @mtias mentioned calling this block "Buttons" any thoughts on the unification of Buttons and Button @mtias?

@paaljoachim
Copy link
Contributor

paaljoachim commented Sep 18, 2019

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.

@mtias
Copy link
Member

Button is expected to become a child block of Buttons, so the inserter would only expose Buttons by default. Just need to ensure nothing weird happens if you use Button directly in a template or existing content.

@jorgefilipecosta
Copy link
Member Author

Button is expected to become a child block of Buttons, so the inserter would only expose Buttons by default. Just need to ensure nothing weird happens if you use Button directly in a template or existing content.

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.

@scruffian
Copy link
Contributor

I tested this and it looks great 👏

@paaljoachim
Copy link
Contributor

Is this being backported to WP 5.3?

@mtias
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract all of this to InnerBlocks horizontalDisplay?

Copy link
Member Author

Choose a reason for hiding this comment

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

I extracted this CSS logic into generic InnerBlocks flags.

@jorgefilipecosta
Copy link
Member Author

I performed some updates on this PR, following some suggestions by @mtias. I moved the movers into the block toolbar as an experiment, and now the standard block UI is hidden using generic inner blocks flags added in #18173.

if ( setting === 'opensInNewTab' ) {
onToggleOpenInNewTab( value );
}
} }
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a stab at this here #19396

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Nice work here 👍

@aduth
Copy link
Member

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

@ellatrix
Copy link
Member

The alignment buttons don't work. See #19616.

Comment on lines +100 to +102
const handleLinkControlOnKeyPress = ( event ) => {
event.stopPropagation();
};
Copy link
Member

Choose a reason for hiding this comment

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

What purpose does this serve? How would a future maintainer be expected to interpret this requirement to avoid a regression?

A few suggestions:

  • Event#stopPropagation is 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.

@cinghaman
Copy link

@aduth do you know if this is going to be added to the core gutenberg anytime soon?

@aduth
Copy link
Member

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button Block and Multiple Buttons