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

Column wrapping is broken between 599px and 782px #18416

Closed
ZebulanStanphill opened this issue Nov 9, 2019 · 11 comments · Fixed by #20597
Closed

Column wrapping is broken between 599px and 782px #18416

ZebulanStanphill opened this issue Nov 9, 2019 · 11 comments · Fixed by #20597
Assignees
Labels
[Block] Columns Affects the Columns Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@ZebulanStanphill
Copy link
Member

The issue

Consider a Columns block containing 2 columns. The Columns block uses flex-wrap: nowrap at a viewport width of 782px and above. Below that, it uses flex-wrap: wrap, which, due to the 32px left margin applied to the 2nd column, would immediately wrap if it weren't for this line applied to the small breakpoint in packages/block-library/src/columns/style.scss:

flex-basis: calc(50% - #{$grid-size-large});

By subtracting half of the margin size from the flex-basis calculations of each column, the columns do not wrap needlessly and everything is fine.

Until you decide to use custom column widths. Custom column widths are set using the style JSX attribute and do not take into account the gap between columns, so when your viewport is between 599px and 782px (exclusive), the columns wrap, but their width does not extend to be 100% like it does at 599px and below.

Here is a screenshot showing the effect of this bug, as well as some of the related CSS rules:
image

Proposed solutions

At first it might seem that the answer is changing the inline style attribute for custom column widths in packages/block-library/src/columns/editor.scss. However, while something like flexBasis: calc(${width}% - 16px) would stop the weird wrapping, it would also cause sets of 3 or more columns to remain side-by-side at widths as small as 600px!

There is also another issue: while 16px works, it's a hardcoded value that should be replaced by the $grid-size-large SCSS variable. But how do you use an SCSS variable in JSX style attributes? Is that even a good idea in the first place?

Looking at the comments in the code and the commit history, I think the intention is that, between the small and medium breakpoints, the Columns block should show a maximum of 2 columns side-by-side, with each column taking up 50% of the available width.

Custom column widths were added later on, and since they use inline styles, they always override the maximum columns rule. Combined with flex wrapping being enabled below the 782px breakpoint, this causes columns with custom column widths to always wrap oddly at this viewport width.

A simpler solution is to use the dreaded !important on the 2 columns rule, forcing columns to all be the same calc(50% - #{$grid-size-large}) width between 599px and 782px viewport widths.

Notably, this would result in odd-numbered last columns taking up half the width of the screen with nothing on the other half. Is this desirable? If not, you could simply add another rule for odd-numbered last columns (e.g. .wp-block-column:last-child:nth-child(odd)) to make them full-width. (Of course, this would also have to use !important.).

One con of this method is that using !important would make theming more difficult; then again, it is already being used by the 1 column rule for viewport widths <= 599px.

How to reproduce

  1. Insert a Columns block.
  2. Insert stuff in the first column.
  3. Insert stuff in the second column.
  4. Save the post.
  5. View the post at a viewport width between 599px and 782px (exclusive).

Environment

  • WordPress 5.3 RC4
  • Gutenberg 6.8.0

Tested on Twenty Nineteen, Twenty Twenty, and Divi. This is not a theme-specific issue.

@ZebulanStanphill ZebulanStanphill changed the title Column wrapping is broken above 599px Column wrapping is broken betweeb 599px and 782px Nov 9, 2019
@ZebulanStanphill ZebulanStanphill changed the title Column wrapping is broken betweeb 599px and 782px Column wrapping is broken between 599px and 782px Nov 9, 2019
@ellatrix ellatrix added [Block] Columns Affects the Columns Block Needs Testing Needs further testing to be confirmed. labels Nov 21, 2019
@ZebulanStanphill ZebulanStanphill added the [Type] Bug An existing feature does not function as intended label Nov 29, 2019
@tellthemachines
Copy link
Contributor

This is an interesting problem! Confirming I can reproduce it locally.

I'd like to propose a solution using CSS grid instead of flexbox. There are several advantages to that approach:

  • All the column widths are defined on the parent element with grid-template-columns, and we can set a specific percentage for one of the columns and let the other(s) automatically take up the remaining space. So we'd have something like this in our stylesheet:
.wp-block-columns {
    display: grid;
    grid-gap: $grid-size-large;
}
.wp-block-columns-2-cols {
    grid-template-columns: repeat(2,1fr);
}

Then if the user decides that the left column should take up 65% of the space, we can add inline grid-template-columns: 65% 1fr. (This is still not perfect, because if we want a single column layout on small breakpoints we'll need to use !important but even that can be solved: as soon as we can start using CSS variables we won't need to rely on inline styles anymore 🎉 )

  • We don't have to manually set margins for our columns, because grid-gap does all the work for us. (For the IE fallback, because IE doesn't support grid-gap, we can add extra narrow columns between the main ones 😛 )

  • All this requires much less/ much simpler code (even counting IE support) than the existing solution.

  • The less styles we have to define on our side, the easier it is for themes to override them.

  • We could potentially introduce a setting where the user picks a fixed column width and the content reflows to as many columns as fit on the screen. Grid does that out of the box!

@aduth
Copy link
Member

aduth commented Mar 2, 2020

I think there's some conveniences in using Grid (notably the gap logic), but I'm not entirely convinced it's the right way to go. At least, my understanding of the distinction between Grid and Flexbox -- and when to use which -- is largely on the basis of 1-dimensional vs. 2-dimensional content. There is a potential model of considering columns as 2-dimensional content, which is in-fact how columns were initially implemented, up until #7234. You can see the specific problems with using Grid that way in that pull request and the original issue at #5351. Introducing the column wrapper element resolved this, but in so doing essentially converted the element hierarchy to one of two containers, where each container presents its contents 1-dimensionally (a Columns of horizontal Column elements, and a Column of vertical block content).

See also: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout/Relationship_of_Grid_Layout#Grid_and_flexbox

The basic difference between CSS Grid Layout and CSS Flexbox Layout is that flexbox was designed for layout in one dimension - either a row or a column. Grid was designed for two-dimensional layout - rows, and columns at the same time.

As I see it, if we're continuing to implement the Column wrapper, then Flexbox seems like the best fit.

To a lesser degree, the current browser support forces us to support Flexbox anyways. It would be a maintenance burden to need to support both.

@aduth
Copy link
Member

aduth commented Mar 2, 2020

Related comment at #20169 (comment) with regards to the original issue reported here:

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
Copy link
Member

aduth commented Mar 2, 2020

An approach using !important is proposed at #20597. On some further evaluation, I sense that this is entirely consistent with how we're already styling columns at mobile viewport ranges, so isn't overly concerning to me as a solution.

@tellthemachines
Copy link
Contributor

@aduth the distinction between 1 and 2-dimensional content when choosing between flexbox and grid is only a guideline. A 1-dimensional design of sufficient complexity, such as what we have, especially when custom column widths come into play, is better served by grid than flexbox. Flexbox works with fixed width contents, but it wasn't designed for that purpose. The other consideration is that our columns reflow on mobile devices, thus becoming 2-dimensional.

The problem with the previous implementation was trying to map the inner blocks of each column to the grid, when we should be mapping only the columns themselves. There might be a place for a Grid block that creates a grid of inner blocks, but that's not how the columns block should behave 😅

The advantages of using grid to solve this problem are the following:

  • less overall code needed
  • the parent (columns block) is in charge of determining the relative widths of its children, which have to resize and reflow when custom widths are set on any of the columns
  • less inline styles as a consequence of the above, because we only need to set them on the parent
  • if we use CSS variables to set the columns template, it becomes ridiculously easy for a theme to override our styles with custom ones, because we can get rid of inline styles and !importants altogether
  • themes can set a custom grid-gap, which is impossible right now
  • themes can determine responsive behaviour, which is impossible right now

If it's acceptable to have the custom column widths editor preview not work on IE, we could do this very neatly with CSS variables and grid, and a basic IE fallback using flexbox and auto widths. If we have to fully support custom widths in IE, that would require a more complex fallback, and use of inline styles which are harder to override. We should bear in mind that even if IE users are not able to preview resizing columns in the editor, it should still be possible for theme authors to create custom IE-compatible column styles if they so desire.

Overall, it seems to me it would be worth trading a little bit less functionality in IE for sizeable benefits in theme styling. I'm happy to build another prototype using grid on top of the current columns implementation and we can see how it works 😁

@ZebulanStanphill
Copy link
Member Author

@tellthemachines I think you should open a separate issue for using CSS Grid in the Columns block. I definitely agree that it's probably the way to go in the long term, though IE11 support is definitely going to be the biggest hurdle to overcome.

@FayeDM
Copy link

FayeDM commented Mar 6, 2020

I was about to submit a ticket for this and then saw this. So I will instead just add my info in case it's useful.

Describe the bug
I discovered a that there is no styling compensation for the margins when a column block has its percentages altered between the views of 599px and 782px.

To reproduce
Steps to reproduce the behavior:

  1. add new page or post
  2. add a Gutenberg Column Block
  3. add full width content to each side (example, an image, or a paragraph block with a background color)
  4. using the block settings, set the percentage width of one block to something other than default. 65% is the test I have been using.
  5. Update
  6. View page on front end
  7. Reduct page size to 700px
  8. see that the margin of the second column is pushing it to the next line.

Expected behavior
I would expect that the flex basis assigned by using the percentages would account for the margins at this view port to prevent the items from being pushed to the next line. If the percentages are left at default, the items would sit naturally next to each other.

Screenshots
Break - what happens when you have percentages set, viewed around 700px.
Column 1 Flex - Flex basis of the first column applied
Column 2 Flex - Flex basis of the second column applied
Default - what happens when there are no percentages set

Desktop (please complete the following information):

  • OS: macOS Mojave
  • Browser: Chrome
  • Version: 80.0.3987.122

Additional context

  • Seen on WordPress 5.3.2

@aduth
Copy link
Member

aduth commented Mar 6, 2020

Hi @FayeDM . If I understand your report correctly, I expect this should be resolved as of #20597, which will be available in the Gutenberg plugin release scheduled for next Wednesday. I'd appreciate if you could give it another try with the plugin once it's released to see if it fixes your issue.

@FayeDM
Copy link

FayeDM commented Mar 6, 2020

Oh, that's wonderful! I will for sure do that.

@glenviewjeff
Copy link

glenviewjeff commented Jun 27, 2020

I'm still seeing this problem myself in the latest version on Gutenberg Drupal. I reported it there; it appears as if they haven't yet merged this fix into their release.

@andymagill
Copy link

I am still seeing this problem on a fresh install of WordPress. The css here is heavy handed and forces complex overrides to be written. Is this in the stable release?

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 [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
7 participants