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 width selector for button block #25999

Merged

Conversation

stacimc
Copy link
Contributor

@stacimc stacimc commented Oct 9, 2020

Description

Issue #24705

Adds a width selector to the sidebar for the Button block, allowing the user to set their button to 25%, 50%, 75%, or 100% of the parent container.

By default, no percentage width is selected and the button width is determined as normal by the size of its content.

Notes

This is an alternative approach to #26045, which implements a width selector on the Button block using a new block support._ This is related to issue #26368.

How has this been tested?

Manually with the button block, using left, right, and center alignments.

  1. Add a Buttons block to a post with one or more Buttons
  2. Select a Button
  3. You should see a Width Settings section under the Inspector Controls, with options for 25, 50, 75, 100%.
  4. Toggle a width option and verify that the correct inline style is applied to the block.
  5. Save the post and view on the frontend.
  6. Verify that the button is resized correctly in the saved post.

Screenshots

button_resize

Screen Shot 2020-10-09 at 1 58 03 PM

Types of changes

New feature (non-breaking change which adds functionality)

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@stacimc stacimc force-pushed the add/width-selector-for-button-block branch from b3853a1 to 9ffdfd5 Compare October 9, 2020 21:18
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Oct 11, 2020

This tested well for me. As mentioned in discussions when using right align things can end up slightly odd, but I think this is because of the use of float:right. When adding width settings to a button block in another project we removed the default right float. @apeatling, I am thinking that it wouldn't be as easy to do that here are existing content may be relying on the existing float:right behaviour?

@aaronrobertshaw
Copy link
Contributor

Code looks good ✨

Apart from the align left/right issues, this tests well for me as well. I do wonder though if this is another opportunity for adding block support for custom widths so other blocks can opt-in and leverage this? This would also allow width options to be configured via themes.

Regarding the alignment issues, I'd vote for them to be addressed in a separate PR. There might be a few things we could do to smooth some of the more common rough edges created by the floated block losing the parent width to calculate the percentage widths from.

@stacimc
Copy link
Contributor Author

stacimc commented Oct 12, 2020

As mentioned in discussions when using right align things can end up slightly odd, but I think this is because of the use of float:right.

This is something I had a lot of difficulty with -- I have a version which I can push up for testing where right-aligning the Buttons block causes the individual Buttons to line up flush to the right side of the container when the post is saved:

Screen Shot 2020-10-12 at 10 20 15 AM

This seems better to me because it is more accurate to what you see in the editor and it's what I would assume should happen when you right-align the parent. The CSS for that removes the float:right like you said and adds display: flex to the Buttons when saved.

here are existing content may be relying on the existing float:right behaviour

I think that's accurate. The reason I had trouble with this is because I couldn't figure out how to best emulate the right-align behavior in production, which does not necessarily keep the buttons flush to the right side of the container. For example, this happens on production when I right-align a group of buttons within a column (column highlighted in blue, sorry it's hard to see):

Screen Shot 2020-10-12 at 10 16 51 AM

@stacimc stacimc force-pushed the add/width-selector-for-button-block branch from 1fd11f6 to 5e57245 Compare October 23, 2020 19:31
@stacimc stacimc marked this pull request as ready for review October 23, 2020 21:05
@apeatling
Copy link
Contributor

@mcsf Pinging you to see if this PR looks better since it's adding width specific to the button block. 👍

@talldan
Copy link
Contributor

talldan commented Oct 27, 2020

There's another PR that changes how alignment works on the buttons block, so it'd probably worth testing against that as well - #23168

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Looks good overall (I left some questions on the attribute schema to make sure we've given this a bit of thought), but I'd have someone else test and review the style changes.

packages/block-library/src/button/edit.js Outdated Show resolved Hide resolved
@@ -52,6 +52,9 @@
},
"gradient": {
"type": "string"
},
"width": {
"type": "number"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about possible futures for this block, is this the best data representation? How would we express custom width values without introducing compatibility issues with the current schema? That is, this new width attribute is not any number but actually a percentage represented as 0 <= n <= 100. Would it be better if it were a real number x 0 <= x <= 1 to distinguish from full pixel values like width: 200? Would it be better if termed widthPercentage?

All of these are open questions — I don't have a formed opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another possibility here would be to add an additional widthUnit attribute if we decide to introduce custom width as pixel values. The search block takes that approach.

Copy link
Contributor

@apeatling apeatling Oct 29, 2020

Choose a reason for hiding this comment

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

This is the approach the search and cover blocks take, so this should be futureproof for adding a unit in the future.

@stacimc
Copy link
Contributor Author

stacimc commented Oct 27, 2020

Thanks for the heads up about PR #23168, @talldan. I pushed up a small change to allow for the larger percentage selections in the new full-width Buttons container.

Staci Cooper and others added 7 commits October 28, 2020 10:07
Initial commit adding a width option for the Button block with
options at 25, 50, 75, 100% widths. By default none are applied
and the button is sized by its content.
A width selection can now be toggled off by clicking the already
selected option.
PR WordPress#23168 overhauls alignment controls on the Buttons container and
allows for full width. For a Button inside a very large container,
in order to be set correctly to 100% it must be able to exceed any
configured max-width.

This commit does not remove the max-width for any button that does
NOT have a custom width percentage selected. This is so that
default buttons continue to be sized as they normally would.
Copy link
Contributor

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

This looks good and tests well. I think this is a great start to width support that could be extended in the future to support custom widths and different units.

Fixes a bug where the inner button was not being set correctly to 100%
width on the frontend, due to the missing classname.`
@stacimc stacimc force-pushed the add/width-selector-for-button-block branch from e7100b2 to eb894de Compare October 29, 2020 21:34
Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

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

This tested well for me.

@apeatling
Copy link
Contributor

@mcsf Would you be able to give this one a sign off?

@apeatling
Copy link
Contributor

Going to move ahead with this once since it has approval already. If we need to make further changes the we can open follow up PRs.

@apeatling apeatling merged commit fb1cc0e into WordPress:master Nov 3, 2020
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 3, 2020
@github-actions
Copy link

github-actions bot commented Nov 3, 2020

Congratulations on your first merged pull request, @stacimc! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@kebbet
Copy link
Contributor

kebbet commented Feb 17, 2021

Is there a way to disable this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants