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

@jorgefilipecosta jorgefilipecosta commented Sep 5, 2019

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

@mapk mapk commented Sep 12, 2019

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

@jorgefilipecosta jorgefilipecosta commented Sep 12, 2019

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 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
Contributor

@mtias mtias commented Sep 22, 2019

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.

@scruffian scruffian mentioned this pull request Sep 23, 2019
5 tasks
@jorgefilipecosta
Copy link
Member Author

@jorgefilipecosta jorgefilipecosta commented Sep 23, 2019

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

@scruffian scruffian commented Sep 24, 2019

I tested this and it looks great 👏

@paaljoachim
Copy link
Contributor

@paaljoachim paaljoachim commented Sep 24, 2019

Is this being backported to WP 5.3?

@mtias
Copy link
Contributor

@mtias mtias commented Sep 24, 2019

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
Contributor

@mtias mtias Sep 24, 2019

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

@jorgefilipecosta jorgefilipecosta Nov 1, 2019

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 jorgefilipecosta force-pushed the add/buttons-block branch 2 times, most recently from b279dda to 90c1f46 Oct 29, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/buttons-block branch 2 times, most recently from c398b47 to e364932 Nov 1, 2019
@jorgefilipecosta
Copy link
Member Author

@jorgefilipecosta jorgefilipecosta commented Nov 1, 2019

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

@youknowriad youknowriad Jan 3, 2020

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

@youknowriad youknowriad Jan 3, 2020

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

Nice work here 👍

@jorgefilipecosta jorgefilipecosta merged commit a74921d into master Jan 3, 2020
2 checks passed
@jorgefilipecosta jorgefilipecosta deleted the add/buttons-block branch Jan 3, 2020
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@aduth
Copy link
Member

@aduth aduth commented Jan 6, 2020

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

@ellatrix ellatrix commented Jan 14, 2020

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

const handleLinkControlOnKeyPress = ( event ) => {
event.stopPropagation();
};
Copy link
Member

@aduth aduth Jan 14, 2020

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

@cinghaman cinghaman commented Jan 15, 2020

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

@aduth
Copy link
Member

@aduth aduth commented Jan 15, 2020

@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