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

Block Library: Columns: Fix equal column growth for unassigned-width columns #20169

Merged
merged 4 commits into from
Feb 11, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 11, 2020

Introduced in: #19515

This pull request seeks to resolve a styling issue introduced in the changes of #19515 (5d9dff2) where columns which do not have an explicitly-assigned width will be allowed unrestrained growth disproportionately to other columns in the set.

Before After
Before After

The intended behavior of columns without assigned width is that they should occupy the available space equally with other columns without assigned width. For example, a newly inserted three-column block should behave such that each of the three columns occupy a third of the available space. The changes in 5d9dff2 allowed for growth, but also allowed for one column to grow more than the others.

The changes here seek to address this by reintroducing a flex-basis as a default style for columns without one of its own. In order for available space to be divided equally amongst columns without an assigned width, this value must be consistent (equal), not cause the sum total of column widths to exceed 100%, and cede to a column with an assigned width. Thus, a flex-basis: 0 is chosen which, in combination with the existing flex-grow: 1, should allow columns to maximally and equally occupy space remaining after subtracting the space occupied by columns with explicit widths (if any exist).

Testing Instructions:

Verify that a default columns block of three equal columns should result in columns which each occupy a third of the available space.

  1. Navigate to Posts > Add New
  2. Insert a Columns block
  3. Choose the "Three columns; equal split" variation when prompted
  4. Insert a paragraph block in one of the columns
  5. Add enough text that wrapping occurs.
  6. Note (both in the editor and in the preview) that each column occupies a third of the available space.

Try with different variations of unassigned and assigned width columns:

  • 25% | auto | 25%
  • 20% | auto | 20% | auto
  • auto | auto | 33%
  • etc.

Note that the widths between two columns arrangements 25% | 50% | 25% and 25% | auto | 25% are similar but not identical. This is impacted by the "gutter" margins between each column. My understanding is that the behavior is correct, where the implication of "auto" to "occupy available space" is affected by the fact that "available space" will be after margins are deducted, whereas with each of the fixed-value widths (25% | 50% | 25%), the widths are calculated before margins are considered, and thus each of the three must be adjusted to offset the margins. You can see that by eliminating margin styles (either in code or by element inspector), the widths of columns in these two arrangements are the same. Note that I don't expect this to be a common consideration for end-users, but including for exhaustiveness' sake in case the disparity is noticed.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention [Type] Regression Related to a regression in the latest release [Block] Columns Affects the Columns Block labels Feb 11, 2020
@aduth aduth added this to the Gutenberg 7.5 milestone Feb 11, 2020
@aduth aduth requested a review from jasmussen February 11, 2020 15:00
@jasmussen
Copy link
Contributor

This is good, it fixes the biggest issue with the content of columns expanding or contracting. That part is solid, I tried breaking it in many ways, with resized images or wordswithoutspaces, it's solid.

There's one issue and I'm not sure it's part of this PR or in master or whether it worked before, but when the viewport becomes < 782px, the 3rd column in a 3 column layout loses its left margin. I.e. > 782 is this:

Screenshot 2020-02-11 at 16 18 56

< 782 is this:

Screenshot 2020-02-11 at 16 19 02

I believe, testing my own site with an older version of the plugin, that 3+ column layouts are meant to collapse into 2 columns at that breakpoint, therefore making the margin unnecessary.

@ZebulanStanphill
Copy link
Member

@jasmussen I don't know if the bug still exists in master as of the most recent changes, but it's worth noting that there has been an issue with columns breakpoints for a while now. See #18416.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Thank you for the fast work.

This is good. I can't break it in my testing, with your latest commit. I even compared the same markup with my site running an older version of the plugin, the behavior is the same.

The 2 column mid-breakpoint is arguably a bit misguided (including by my past self), but things like #19909 depending on how that plays out, could represent an opportunity to revisit and refactor that in the future.

In the mean time, this fixes columns up nicely!

@aduth
Copy link
Member Author

aduth commented Feb 11, 2020

Thanks @jasmussen and @ZebulanStanphill . I overlooked (both here and in #19515) that we have this mid-range viewport styling for columns where we show two equal-width columns. The revisions in 5d9dff2 and in the first commit of this pull request did not account for this, and were modifying styles within the break-small media query that would unknowingly impact this viewport range.

The simplest solution for me is: Restore the styles as they were prior to 5d9dff2 for the mid-range viewport, and apply the changes intended between 5d9dff2 and c1e2c3d as specific to large viewports (break-medium).

This way, the styles in this viewport range should behave exactly as they did prior to #19515.

That being said, as seen with #18416, the mid-range viewport styling still has issues that should be resolved. I don't personally see them as within the scope of what I intend to address here, so I would not suggest to include them. But in consideration of those issues, I see that we should separately consider:

  • It might be worth eliminating the two-column layout, and have only the single-column and horizontal ("original") layouts for columns for small and larger viewports respectively
  • Otherwise: @ZebulanStanphill 's suggestion in Column wrapping is broken between 599px and 782px #18416 to use !important to force the two-columns of the flex-basis is probably valid, and still valid after these changes
    • Perhaps made complicated by the fact that it must only take effect between small-medium so that large viewport flex-basis styling default can take effect without !important. It would probably need to be made as a custom media query, not using existing mixins.

@aduth aduth merged commit 59d7463 into master Feb 11, 2020
@aduth aduth deleted the fix/column-size branch February 11, 2020 16:56
ellatrix pushed a commit that referenced this pull request Feb 12, 2020
…columns (#20169)

* Block Library: Columns: Fix equal column growth for unassigned-width columns

* Block Library: Columns: Restore mid-range viewport styles

* Block Library: Columns: Keep margin resets in mid-range viewports

Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75

* Block Library: Columns: Update comment to reference mid-range 2 columns
@jorgefilipecosta jorgefilipecosta added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 14, 2020
jorgefilipecosta pushed a commit that referenced this pull request Feb 17, 2020
…columns (#20169)

* Block Library: Columns: Fix equal column growth for unassigned-width columns

* Block Library: Columns: Restore mid-range viewport styles

* Block Library: Columns: Keep margin resets in mid-range viewports

Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75

* Block Library: Columns: Update comment to reference mid-range 2 columns
jorgefilipecosta pushed a commit that referenced this pull request Feb 17, 2020
…columns (#20169)

* Block Library: Columns: Fix equal column growth for unassigned-width columns

* Block Library: Columns: Restore mid-range viewport styles

* Block Library: Columns: Keep margin resets in mid-range viewports

Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75

* Block Library: Columns: Update comment to reference mid-range 2 columns
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants