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

[WIP] Update Columns block to use CSS Grid, add option to adjust grid gap #32137

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented May 24, 2021

Description

This is currently a work in progress, but the goal is to explore refactoring the Columns block to use CSS Grid, and add in support for grid gap, to allow a simple control at the parent Columns block to adjust the spacing between column blocks.

I attempted to use grid template columns to avoid having to use media queries, however for grids that are non-uniform, we can't use repeat( auto-fit, ... ) because each column needs to have its own width. To support non-uniform grids, at larger viewports we attempt to respect the column width, then at intermediate / mobile widths we collapse down to two columns, and then one on mobile. This needs further tweaking to get it feeling "right".

Related to #30753

Still to do:

  • Add workaround for issue in Safari where changing grid-gap CSS values does not result in the gap being re-rendered (0e0a19b)
  • Add / update deprecations
  • Add server-side approach to insert styling (addresses deprecated markup that expects the flex approach, and avoids having to add a deprecation)
  • Add / fix up JS tests
  • Add PHP test for server rendering
  • Test that this works across browsers
  • Ensure the styling on the front end of the site doesn't result in styling regressions for existing columns out in the wild (done, but needs testing to confirm)
  • Test and get the gap working on native mobile

How has this been tested?

TBC

Screenshots

columns-gap-sml

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@andrewserong andrewserong changed the title [WIP] Refactor Columns block to use CSS Grid, and add option to adjust grid gap [WIP] Update Columns block to use CSS Grid, add option to adjust grid gap May 24, 2021
@andrewserong
Copy link
Contributor Author

Added a change in 0e0a19b that updates an unused CSS attribute to force Safari to re-render the columns. In Safari, there appears to be a bug where changing the one grid-gap property on its own does not cause Safari to redraw the layout. A workaround is to update any other CSS property that does cause Safari to redraw, or to force a DOM update in some other way by altering markup further down the tree (such as when we click into a child block). To provide a simple and hopefully innocuous workaround, I've used the counter-reset CSS property as the property to change to force a redraw. Happy to explore other approaches if this hack doesn't seem appropriate, though (but I was pleased to get a hack in that was adding only a single line).

@jasmussen
Copy link
Contributor

Thanks so much for exploring this. I already love the amount of red here.

I see some 0.9999fr units in the code, any reason they can't be 1fr? Are they calculated variables based on the 60/20/20 percentage values I entered here?

collapsing

The above looks great, by the way. It looks like the flex basis is used beyond a breakpoint, and that the grid stacking happens below that breakpoint?

I attempted to use grid template columns to avoid having to use media queries, however for grids that are non-uniform, we can't use repeat( auto-fit, ... ) because each column needs to have its own width.

Right, really valuable observation, thank you. I'll look into this a bit further, I feel like this should be solvable. Something like if above this minmax combo, allow the flex basis to take. If below that, stack.

It's a bit of a dream scenario, but the chief benefit of doing it so is that it can save us from a potentially complicated responsive editing interface. Instead of designing for individual breakpoints, we design behaviors that work for any breakpoint. Thanks for working on this, and let me know how I can help!

@andrewserong
Copy link
Contributor Author

Thanks for giving this a test @jasmussen, much appreciated!

I see some 0.9999fr units in the code, any reason they can't be 1fr?

Yes, it's calculated based on the width attribute, where specified of the child column. If a width attribute isn't present, it'll default to 1fr for each column.

I've added in a call to toWidthPrecision so these numbers will be rounded to 2 decimal places now. For a 60/20/20 layout, this now appears to get the correct 1.8fr 0.6fr 0.6fr values we'd expect, and on my other test columns it looks like it's rounding to 1fr instead of 0.9999fr.

image

It looks like the flex basis is used beyond a breakpoint, and that the grid stacking happens below that breakpoint?

I believe the flex-basis value of the child columns is ignored, since the parent is set to display: grid. We then use the width attribute for calculating the values for grid-template-columns which is overridden on mobile / smaller viewports by the setting in the media query. It doesn't appear to hurt having the inline flex-basis value, but if this PR looks viable, I can add in a deprecation to remove the flex-basis value to keep things tidy.

I'll look into this a bit further, I feel like this should be solvable. Something like if above this minmax combo, allow the flex basis to take. It's a bit of a dream scenario, but the chief benefit of doing it so is that it can save us from a potentially complicated responsive editing interface. Instead of designing for individual breakpoints, we design behaviors that work for any breakpoint.

Yes, I'd love it if we can work out a way to avoid the break points. I haven't managed to get it working from my hacking around... it appears as though the magic auto-fit setting for repeat doesn't let us vary the width of each individual column, but treats each as an equal. And without auto-fit, setting the grid explicitly via grid-template-columns: 1.8fr, 0.6fr, 0.6fr; tells the grid that we always expect three columns of these proportions, with no wrapping.

Thanks for working on this, and let me know how I can help!

My pleasure. If you can think of any other things to try for avoiding media queries with these non-uniform layouts (e.g. 30/70, 70/30, 25/50/25, etc), that'd be a big help! I haven't had much luck so far, and while we can always proceed without it and explore in a follow up, it'd be good to dig in a bit further and see what our options are 🙂

@andrewserong
Copy link
Contributor Author

In 6ef33f6 I've:

  • Moved the calculations to a utility function and added tests for it
  • Updated all the deprecations for the Columns block to add the correct attributes
  • Moved the fixtures for the deprecations around slightly and updated them
  • Added an additional deprecation and fixture

The above probably needs a bit more work, but mostly covers deprecations in the editor for the moment. I still need to add in probably a PHP approach to add in the styling server-side for deprecated markup that hasn't yet been migrated (e.g. older posts). I'll take a look at that tomorrow.

I'm also wondering if we need to use grid-gap instead or in addition to the gap CSS attribute, given that grid-gap has better support on earlier versions of some browsers 🤔

@jasmussen
Copy link
Contributor

As an experiment, this is definitely interesting, thanks again for looking. I think it might be worth soliciting some more takes to see what the pros and cons are; to me being able to retire the giant list of CSS width rules to accommodate what the gap rules can also do, that alone seems worth it.

I'll definitely look at whether we can find an alternative to media queries that allows stacking on mobile but still arbitrary width columns on desktop. The other great reason to remove them is that they'd stack based on container width, rather than viewport width, which is key for patterns. But if all fails, we can look to container queries.

CC: @kjellr if you have thoughts on gap vs grid-gap, per Andrew's question here:

I'm also wondering if we need to use grid-gap instead or in addition to the gap CSS attribute, given that grid-gap has better support on earlier versions of some browsers 🤔

@jasmussen
Copy link
Contributor

The example below font-size on this codepen is the closest I've gotten, so far, to variable-width columns. In that example, it means we have to drop the use of percentages, and embrace instead column-span and more pure CSS grid, so for example have a 12 column grid where 3 columns inside distribute themselves neatly and fill 3 columns if that's all there is, but then you can set the first of those three to, say, span at most 6 but at least 4. It might enter territory that my friend @vcanales has explored also.

@kjellr
Copy link
Contributor

kjellr commented May 26, 2021

I'm also wondering if we need to use grid-gap instead or in addition to the gap CSS attribute, given that grid-gap has better support on earlier versions of some browsers 🤔

My understanding is that all the grid- prefixed properties have been depreciated, as per the MDN docs. I'm not sure there's much value in using the old ones here, since we're already using non-prefixed gap values elsewhere (for the button block).

@andrewserong
Copy link
Contributor Author

Thanks for confirming about using gap Kjell!

I think it might be worth soliciting some more takes to see what the pros and cons are; to me being able to retire the giant list of CSS width rules to accommodate what the gap rules can also do, that alone seems worth it.

@jasmussen agreed! Very happy for more feedback if there are other folks who might have an opinion here.

In that example, it means we have to drop the use of percentages, and embrace instead column-span and more pure CSS grid, so for example have a 12 column grid where 3 columns inside distribute themselves neatly and fill 3 columns if that's all there is, but then you can set the first of those three to, say, span at most 6 but at least 4.

That's a really interesting approach Joen! A hard part about going with a 12 column effective layout would be if we have any users using a non-standard layout that doesn't neatly conform to 12ths (e.g. a 40% / 60% layout, fifths, or our 60/20/20 example). For existing potential users and patterns out in the wild, I imagine that could be a deal-breaker? I can't wait for real container queries, of course!

I'll continue exploring our options in this PR. Next up I'll take a look at a PHP approach for dealing with deprecated markup that hasn't been migrated (which we'll need to deal with, whichever CSS approach we use, I think).

@andrewserong
Copy link
Contributor Author

Update: I'm not quite sure if it's the right approach, but I've added in server rendering for the block, that calculates the grid-template-columns value in PHP for rendering on the front end of the site. This addresses the issue of deprecated markup out in the wild that hasn't been migrated. While working on it, I realised that we could use the server-rendered approach in all cases so that we can avoid having to add a deprecation for the block. This simplifies the attributes a little (we're only adding in attributes for the grid gap), but the main benefit is that, a bit like the color block support feature, this gives us the flexibility to change the grid CSS further down the track without having to deprecate and migrate post content.

As we're likely to continue to iterate on the approach to CSS grid, this seemed a bit safer to me than baking the styles into post content, particularly if we change direction through the lifecycle of this PR, or after it lands (if this approach looks viable, of course).

There's a bit of double-up of logic here (we need to replicate the logic in both JS and PHP), and I still need to add some PHP tests. I'll continue on with that next week.

@andrewserong
Copy link
Contributor Author

I've added in a control for updating the grid gap in native mobile in 18fd3d2 — the control is there, and it correctly updates the block's attributes. Still to be done, is reflecting this in the rendering of the blocks within the editor canvas.

Just in case anyone else is looking at this PR, I'm going to be AFK the rest of this week, so I'll come back to this PR on Monday.

@jasmussen
Copy link
Contributor

Also wanted to surface #32366 as adjacent to this conversation!

@andrewserong
Copy link
Contributor Author

Update: I think exploring the gap block support is a terrific idea. I've added to the discussion over in #32366 and started a draft PR to explore that in #32571.

@kjellr
Copy link
Contributor

kjellr commented Jun 24, 2021

Tested this out today, and it seems to work pretty nicely! The ability to adjust the gutter between columns would really come in handy for block patterns. What's left do before this is viable?

This would fix close out #10730

@andrewserong
Copy link
Contributor Author

Thanks for testing this out! The main thing that’s left to do is to first resolve the generalised gap support tracked in #32366 — there are a couple of other PRs for the spacing controls that I believe need to be merged first, then we can proceed with the gap support and once that lands roll it into this. I believe that approach is preferable to landing this PR in its current state so that we can avoid adding more deprecations once the gap support is in place. I’ve been a bit side tracked by other work the past couple of weeks, but hope to get back to this and the related PRs next week.

@aristath
Copy link
Member

There's 2 things I don't like with the current implementation and I've been trying to find solutions to these issues for quite some time:

  • media-queries and arbitrary breakpoints
  • margins

I've used CSS-grid for the columns block in one of my themes and though it worked, it took a lot of hacking and a lot of CSS to finally get all scenarios covered. In the end, it wasn't a solution that I'd like to see in core as it ended up adding more complexity to the mix.

I love CSS-grid... but I've come to believe it's not the right tool for the job at hand when it comes to the columns block.
The main difference between flex and grid, is that flex works on 1 axis while grid works on 2 axis. In other words, in grid we have both rows and columns while in flex we only have rows.
In the columns block what we want the browser to do is to fill in a row, then if the items don't fit create a new row, fill it in, and so on. When the browser is calculating things on row 2, it should not be affected by the size of items in row-1. So we are only concerned with the X axis and not the Y. In css-grid, if we have 3 columns and the 3rd one is on a 2nd row, then it should span 2 columns and not 1. That's easy with flex 'cause the browser doesn't know what went on in row 1 when it's working on row 2. But it's not as easy in css-grid... not without adding complexity to the css, complexity that we want to resolve.

The one true benefit of css-grid over css-flex for our implementation is the use of gap: grid-gap is widely supported in browsers. Flex-gap is also a thing and right now supported by all browsers, but it was only recently added in Safari which makes it problematic for old iPhones & OSX versions. But it can be polyfilled...

My latest experiment on #33330/files is still a POC/WIP and doesn't work quite right yet (it works fine on the frontend I just haven't implemented the editor-side yet), but shows a different concept: It gets rid of the media-queries and introduces a "minimum column width" control in the columns block.
Perhaps we could combine the 2 concepts somehow and come up with a solution that is greater than the sum of its parts?

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Nice work! I left a few comments and suggestions below, but I think this is a good approach to solving the problem. As I see it, there are two big advantages to using Grid instead of Flex:

  • Better gap support allows for setting custom gap width
  • Same (or better) results as Flex implementation with a great deal less code 😁

Grid was created with two-dimensional layouts in mind, whereas Flex is for one-dimensional, but that doesn't change the fact that Grid works better than Flex at solving most one-dimensional layout problems too. Having the container set widths and responsive behaviour for its children is so much simpler and easier to control than having to set the width in each child separately (even if that's what we do in the UI).

In that example, it means we have to drop the use of percentages, and embrace instead column-span and more pure CSS grid, so for example have a 12 column grid where 3 columns inside distribute themselves neatly and fill 3 columns if that's all there is, but then you can set the first of those three to, say, span at most 6 but at least 4.

I'm not sure about this, as in theory there's no upper limit to the amount of columns a Columns block can house. E.g. I should be able to do:

Screen Shot 2021-07-15 at 12 55 58 pm

(sorry about the horribly clashing colours 😅, I was trying to make it obvious that there are 14 columns with no gaps between)

Lastly, about media queries: I wouldn't worry too much about trying to implement super-complicated grid template, or other logic in order to avoid them. Container queries are being worked on as we speak, and once they ship, it'll be easier to just switch from media queries to container queries.

"type": "number",
"default": 2
},
"gridGapUnit": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need gridGap and gridGapUnit as two separate properties, or can we consolidate them into a string like we do with the width attribute in the Column block (it also uses UnitControl so very similar problem).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think we do. Separately to this PR we're looking at a generalised gap block support (tracked in #32366), which I think will be better than storing the attributes with the block here.

@@ -0,0 +1,92 @@
<?php
/**
* Server-side rendering of the `core/columns` block.
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to server render this block. Usually we only do server-side rendering for blocks involving posts, categories, navigation etc. - stuff we need to grab from the database. Changing styles and adding a couple attributes should only require a deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a bit stuck on this one, because we need to handle published blocks that haven't yet been migrated to the new markup. I'd prefer if we can avoid the server rendering, but unfortunately couldn't quite think of an alternative to ensure blocks out in the wild don't regress 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it should work fine with just a deprecation, and perhaps some adjustments in the editor logic for converting legacy % units to fr and stuff like that. Any specific examples of regressions you detected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember correctly, the steps were:

  • Prior to this PR, publish an existing columns block with some custom widths
  • Apply this PR
  • Before going to the editor, view the columns block on the front end of the site, and notice that the styling no longer works, because we don't yet have the grid-template-columns attribute in the block markup.

Once the block is opened in the editor, and the markup has been migrated, it's fine, it was just about trying to ensure that we either have CSS that can target both versions of the markup (which this PR doesn't have because flex-basis doesn't do anything when we're using CSS grid), or use server-rendering to get it in there.

Happy to take another look, though!

// In viewports smaller than large, try to show 2 columns,
// reducing to a single column in smaller containers.
@media (max-width: #{ ($break-medium - 1) }) {
grid-template-columns: repeat(auto-fit, minmax(240px, 1fr)) !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

In my previous attempt to use grid with Columns, I managed to work around having to override the inline styles with !important by leveraging CSS variables. By setting only a variable value as inline style, it can be changed in the stylesheet without needing !important. Perhaps we could try that approach here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Happy to give that a shot.

@andrewserong
Copy link
Contributor Author

Thanks for the feedback, folks! I'm just at the end of my day, so I haven't had a chance to digest your comments fully yet. From my perspective, my main goal is to help us get to having a grid gap that folks can set within the editor UI. I'm not too fussed personally if we wind up using the gridGap or gap CSS properties directly (and therefore if we use flex vs CSS grid), but just that we can (hopefully cleanly) apply that to the columns block, and CSS Grid seemed like a good thing to explore. I'm very happy to see (and possibly go with) other PRs if another approach becomes more viable.

I'll be on leave for a week after tomorrow, so apologies if there are more replies and I miss them, but I will get back to them eventually! Before this PR (or an alternate approach) is ready to land, I'd like to see us use a generalised grid gap block support, if possible (tracked in #32366). I've done some work on this in #32571 (which is currently blocked on the dimensions panel work). And there's an axial control in the BoxControl component that's ready for us to use that was merged in #32610, too, when we're ready.

Thanks again for taking a look at this WIP! 🙇

@annezazu annezazu added [Block] Columns Affects the Columns Block [Type] Enhancement A suggestion for improvement. labels Aug 25, 2021
@andrewserong
Copy link
Contributor Author

Thanks again for the feedback on this PR everyone, since July work has progressed on the gap block support in a few different places. The gap block support has now been merged in, in #33991 and there's a separate PR to switch the columns block over to using the blockGap value in #34456. Once the latter lands, the next step will be to opt the Columns block into the gap block support.

I'll close out this PR now, as the approach is superseded by the above PRs, but the discussion here will still be useful to refer back to if and when we want to refactor the Columns block. 🙇

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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants