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

Include button margins in width calculations #26781

Merged

Conversation

stacimc
Copy link
Contributor

@stacimc stacimc commented Nov 6, 2020

Description

#25999 added a width selector for the button block, allowing a Button to be set to 25%, 50%, 75%, or 100% width. Margins were not included in the width calculation in the initial iteration, meaning that for example two 50% buttons will not fit on the same line.

This PR includes margins in the width calculation.

How has this been tested?

Manually with the Buttons block, testing with left/center/right alignments. Tested using Seedlet and Twenty Twenty.

Testing Instructions

Add a Buttons block with several buttons and change their width using the width selector in the Inspector Controls.

  • Test that expected widths fit together in a row (four 25%, two 50%, a 50% and two 25%, a 75% and a 25%...)
  • Test that multiple buttons with width 100% are correctly aligned
  • Create a grid of buttons by creating multiple rows with widths adding up to 100%, and verify that the edges of the grid are flush (see screenshots)
  • Test button grids when the Buttons block is set to different alignments (right aligned, center aligned...)

Screenshots

Screen Shot 2020-11-06 at 3 00 08 PM

Screen Shot 2020-11-06 at 1 58 04 PM

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

Fixes #26702

Staci Cooper added 2 commits November 5, 2020 12:10
This allows, for example, two 50% width buttons to fit next to
each other on a single row. Left and right margins are removed
for buttons with width at 100%.
Including the margins even for buttons with 100% width allows
them to line up correctly with buttons of other widths.

For example, two 50% buttons and one 100% button will take up two
rows. For the second row with the 100% button to be flush with
the row above it, it must also include a margin.
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Nov 8, 2020

This mostly works for me except for some weird wrapping of text with longer button labels, eg. with 4 x 25% buttons:

Screen Shot 2020-11-09 at 11 50 09 AM

I am not sure what the preferred behaviour should be in a situation like this.

@stacimc
Copy link
Contributor Author

stacimc commented Nov 9, 2020

This mostly works for me except for some weird wrapping of text with longer button labels, eg. with 4 x 25% buttons:

I agree this looks strange, but I'm not sure how we could handle this in the UI either. Since the width options are explicit user selections, I would not expect the button itself to resize to fit content. I would expect a 25% width selection to be exactly 25%.

The word wrapping within the buttons is consistent with what we already see, for example if you add a button inside a small container like a column:

Screen Shot 2020-11-09 at 10 23 30 AM

Allowing explicit selection of small button widths definitely makes it a lot easier to create these kinds of issues. I'm not sure the UI should prevent the user from doing it.

@stacimc stacimc marked this pull request as ready for review November 9, 2020 18:49
@apeatling apeatling self-requested a review November 9, 2020 19:35
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.

Tested and confirmed working. Buttons now sit on the same line as shown in the description images.

I don't think the text wrapping should be considered as part of this, it would need further consideration on how to expose this in the UI or let themes set the way this wraps.

@apeatling apeatling merged commit 806ccd0 into WordPress:master Nov 9, 2020
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buttons Block: Horizontal margin issue
3 participants