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

Apply inline-flex to all buttons within a ButtonGroup #16686

Merged
merged 1 commit into from Jul 24, 2019

Conversation

@chrisvanpatten
Copy link
Member

commented Jul 19, 2019

Description

Fixes #16664 by applying inline-flex to all buttons within a ButtonGroup (<Button>, <IconButton>, etc.)

How has this been tested?

Tested manually in browser, using the following code to insert two ButtonGroups:

var el = wp.element.createElement;
var registerPlugin = wp.plugins.registerPlugin;
var ButtonGroup = wp.components.ButtonGroup;
var IconButton = wp.components.IconButton;
var Button = wp.components.Button;
var PluginPostStatusInfo = wp.editPost.PluginPostStatusInfo;

var ButtonGroupTest = function () {
	return el(
		PluginPostStatusInfo,
		null,
		el(
			ButtonGroup,
			null,
			el(
				Button,
				{
					isDefault: true,
				},
				'1',
			),
			el(
				Button,
				{
					isDefault: true,
				},
				'2',
			),
			el(
				Button,
				{
					isDefault: true,
				},
				'3',
			),
		),
	);
};

registerPlugin( 'button-group-test', { render: ButtonGroupTest } );

var IconButtonGroupTest = function () {
	return el(
		PluginPostStatusInfo,
		null,
		el(
			ButtonGroup,
			null,
			el(
				IconButton,
				{
					icon: 'smiley',
					isDefault: true,
				},
			),
			el(
				IconButton,
				{
					icon: 'smiley',
					isDefault: true,
				},
			),
			el(
				IconButton,
				{
					icon: 'smiley',
					isDefault: true,
				},
			),
		),
	);
};

registerPlugin( 'icon-button-group-test', { render: IconButtonGroupTest } );

Screenshots

Before:
Screen Shot 2019-07-18 at 11 58 39 AM

After:
Screen Shot 2019-07-19 at 12 21 30 PM

Types of changes

Non-breaking CSS change, unless someone expected IconButtons inside ButtonGroups to display vertically :)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@obenland

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

The PR overrides this style introduced in #383. Is that still needed outside of button groups? If not we could just remove that.

@chrisvanpatten

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

@obenland I considered that but ultimately it could mean a sort of “backward compatibility” break for anyone using IconButton and not expecting it to be a block level element. Scoping the change to the ButtonGroup is a bit safer.

Normally I would have preferred to make the change in IconButton but changing a component from block to inline feels like a significant enough change to warrant a more conservative, scoped approach.

@youknowriad
Copy link
Contributor

left a comment

Makes sense to me.

@chrisvanpatten chrisvanpatten merged commit 28c7f3a into master Jul 24, 2019

1 of 14 checks passed

Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@chrisvanpatten chrisvanpatten deleted the fix/16664-iconbutton-group branch Jul 24, 2019

@aduth aduth added this to the Gutenberg 6.2 milestone Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.