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

Columns block: Add stack on mobile setting to allow for columns without mobile breakpoints #31816

Merged

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented May 14, 2021

Proposal to fix #31248

Description

This PR adds a stack on mobile setting to the Columns block, set to true by default, so that it can be disabled for cases where we don't want the stacked behaviour. Good examples could include logo / social media links in a footer or header.

  • Add a "stack on mobile" setting to the Columns block, and default to true
  • Re-arrange the CSS for the Column block so that it's targeted as a child of the Columns block, and set the mobile breakpoint styling to apply only when the .is-not-stacked-on-mobile class isn't present.

There are some double negatives here, which might be confusing to read, however it helps us to ensure that we treat all existing Column(s) blocks as stacked on mobile, by default.

How has this been tested?

Manually.

  • Add a couple of columns blocks to a post (e.g. a 2 column block and a 3 column block).
  • Test that the existing breakpoints work by default.
  • Toggle the stack on mobile flag, and check that the .is-not-stacked-on-mobile class is being added to the parent Columns block.
  • Publish the post, and test in the editor and in the front end of the site in a range of viewports that the columns are not being stacked

Screenshots

Mobile width Intermediate width Desktop
image image image

Types of changes

Checklist:

  • My code is tested. (Added test fixtures)
  • 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
Copy link
Contributor Author

I've fixed up the deprecations to add in the new attribute, and added another set of test fixtures to cover the class name in the markup (is-not-stacked-on-mobile).

@kjellr
Copy link
Contributor

kjellr commented May 18, 2021

Thanks for the PR @andrewserong! This works well in my testing:

Screen Shot 2021-05-18 at 9 18 34 AM

There are some double negatives here, which might be confusing to read, however it helps us to ensure that we treat all existing Column(s) blocks as stacked on mobile, by default.

These double negatives are my only real hesitation here — you're right that it's confusing to read. I do really appreciate that this works so nicely though. It doesn't break or modify any existing columns at all and that's the most important part. I'd like to cc @jasmussen for a second opinion, but if it seems ok on his end then I think we're good to go.

@kjellr kjellr requested review from jasmussen and kjellr May 18, 2021 15:14
@jasmussen
Copy link
Contributor

One problem with media queries is that they work best in the site editor/iframe, even if there's some query handling in the post editor also. To that end, I've been exploring some container-query-esque alternatives, that I think we might be able to use here as well, in https://codepen.io/joen/pen/wvJMdGr.

Question: if we land this with media queries as is, how are we on the deprecation front if we were to move to a media-query-less alternative as shown above instead? If that's a headache, we might want to postpone adding this feature.

@kjellr kjellr mentioned this pull request May 18, 2021
2 tasks
@Sandstromer
Copy link

I see some strange behavior when the columns are unequal widths.

In the screenshot the 50/50 columns are okay, but the 30/70 and 25/50/25 columns are not.

gutenberg-columns-not-stacked-flex-wrap

It seems that in viewport 782px and above, the columns flex-wrap is nowrap, but this is missing for the .wp-block-columns.is-not-stacked-on-mobile at widths below 782px, and so defaults to the standard responsive flex-wrap: wrap

@jasmussen
Copy link
Contributor

Did you consider the grid based container query approach? Adding display: grid; grid-template-columns: repeat( auto-fit, minmax( 240px, 1fr ) ); gets us relatively far:

grid

The benefit is reduced CSS, working with container widths instead of viewport widths, and moving towards an intrinsically responsive future for the block which I'm starting to think has more potential than a media query based one.

@andrewserong
Copy link
Contributor Author

It seems that in viewport 782px and above, the columns flex-wrap is nowrap, but this is missing for the .wp-block-columns.is-not-stacked-on-mobile at widths below 782px

Thanks for spotting this one @Sandstromer! I've added in the flex:wrap: no-wrap which appears to have fixed that up.

To that end, I've been exploring some container-query-esque alternatives, that I think we might be able to use here as well, in https://codepen.io/joen/pen/wvJMdGr.

Did you consider the grid based container query approach? Adding display: grid; grid-template-columns: repeat( auto-fit, minmax( 240px, 1fr ) ); gets us relatively far:

Really interesting ideas, thanks @jasmussen! And the Logrocket blog post linked through from your codepen is a great read.

It does feel like CSS grid would be a neater approach for this block, and I'm aware my PR adds some additional complexity to the existing styling.

A couple of concerns from the top of my mind in moving to CSS grid would be where are we at with dropping IE11 support on the front end of users' sites? And would that change negatively affect any themes, if they're expecting media queries to be used? I wonder if there's a decent-enough IE11 fallback we can include if need be?

The benefit is reduced CSS, working with container widths instead of viewport widths, and moving towards an intrinsically responsive future for the block which I'm starting to think has more potential than a media query based one.

That's a great point. Particularly for the kinds of patterns we might have in mind, a container based query (or something like it) more closely matches how we'd expect the block to behave. I'm happy to have a go at exploring the CSS grid approach...

Question: if we land this with media queries as is, how are we on the deprecation front if we were to move to a media-query-less alternative as shown above instead?

In terms of potential blockers, since we're not significantly changing the markup for the block, but only adding a single class in this PR, I don't think we'll run into any deprecation issues so long as we maintain the same visual rendering of the block for end users. So if CSS grid support isn't quite there yet, we could always go with what's proposed so far, and iterate to move toward CSS grid in follow-ups, provided we're happy with the additional classname / toggle in this PR.

@jasmussen
Copy link
Contributor

The toggle to stop from collapsing to columns on mobile seems fine to add, so we might pragmatically want to land this after all.

I just think we'll ultimately want to move to CSS grid for columns entirely, and avoid optimizing too much for the current flexbox system. Re-reviewing it seems there's as much red as there is green in this PR. The Media & Text block also adds the additional class, so it's probably fine.

A couple of concerns from the top of my mind in moving to CSS grid would be where are we at with dropping IE11 support on the front end of users' sites? And would that change negatively affect any themes, if they're expecting media queries to be used? I wonder if there's a decent-enough IE11 fallback we can include if need be?

One of the things that excites me the most about the recent decision to drop of support for IE11 is how much leaner and smarter we can build things in the future. Things like gap can likely remove a ton of cruft from the columns CSS. That is perhaps what got me the most excited about moving towards it even now. (Also as it turns out, Media & Text already uses CSS grid).

That said, I recognize that we'll probably be fine with this PR as an interim step on the path to bigger and better things. Thanks for the effort and for entertaining a refactor!

(That said, if we can avoid a :not selector, that would be much preferable, as it adds a huge amount of specificity).

@andrewserong
Copy link
Contributor Author

Things like gap can likely remove a ton of cruft from the columns CSS. That is perhaps what got me the most excited about moving towards it even now.

Yes, I'm particularly excited too about potentially using gap for columns as it'd make controls for column spacing much neater than other alternatives. I'd still like to explore the CSS Grid idea before proceeding with the current approach in this PR. I've been delayed a bit due to investigating a high priority bug elsewhere, but I'll come back to this one if no-one beats me to it 🙂

@andrewserong
Copy link
Contributor Author

I've forked from this PR to hack away at the CSS grid approach. I think it seems promising... just saving my progress with this link to my hack WIP (no need for anyone else to look at that yet), but I'll continue on with exploring it on Monday.

@andrewserong
Copy link
Contributor Author

I've opened up a separate WIP PR to look at CSS Grid support as it's a bit different to the idea in this PR. I'll follow up with further discussion over on #32137 — to be able to support non uniform column widths (e.g. 25% 50% 25%) I'm not sure if we'll be able to avoid media queries to break down the layout on smaller widths, unfortunately (but very happy for feedback and ideas, of course!). But I'll continue exploring over in that draft PR.

@kjellr
Copy link
Contributor

kjellr commented Jun 24, 2021

That said, I recognize that we'll probably be fine with this PR as an interim step on the path to bigger and better things. Thanks for the effort and for entertaining a refactor!

I just retested and this does appear to work ok to me. Should we consider merging this while we wait for a broader grid-based solution to land? Or do we feel that #32137 is close enough, and we can add this control to that PR? (I think either way, we're going to end up with some weird :not rules in order to support existing blocks, right?)

@andrewserong
Copy link
Contributor Author

Thanks for giving this one another test! I definitely went down the rabbit hole investigating CSS Grid support for columns, so it might be more pragmatic to rebase and clean up this PR and get it in sooner if thats preferred.

I agree, I think we’re still going to have some awkward rules to get this working, so I’m open to getting this in before some of the others. I’m at the end of my week, but can give this a rebase and take another look next week.

@andrewserong andrewserong self-assigned this Jul 2, 2021
@andrewserong andrewserong added the [Block] Columns Affects the Columns Block label Jul 2, 2021
@andrewserong andrewserong force-pushed the add/columns-stack-on-mobile-option branch from 08d4199 to 2076c72 Compare July 2, 2021 06:11
@andrewserong
Copy link
Contributor Author

I've given this one a rebase and re-tested, I think it's still working as intended, if anyone would like to give it a review, and if we think this PR is an acceptable change (with all the not:s!) before we refactor the Columns block to use CSS grid.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

This works great in my testing, in both the front and back end:

Desktop Mobile
Screen Shot 2021-07-02 at 2 43 30 PM columns-mobile-editor

I'm ok with the :not() rule, and it would be great to have this merged in while we continue to work on trying out grid. I'd love to see if @jasmussen or someone else can give this one last gut check before merge though.

Thanks for your work on this, @andrewserong!

@jasmussen
Copy link
Contributor

Took it for another spin:

stack

While I still think there's huge potential in grid and gap in the future, this does seem like a pragmatic and relatively safe step forward that as shown solves a bit of an issue present.

I still think the :not is slightly risky. Lately I've become keenly aware that we need to be very careful with block style specificity, given how global styles works. While there aren't that many column specific styles surfaced in global styles at the moment (and I'm not sure what we'd want to have in the near future, other than gap perhaps), just as a general practice I've come to want to default to as low specificity as possible, even leveraging :where for minimal specificity where possible.

Looking at this one: &:not(.is-not-stacked-on-mobile) it's a double negative and a bit of a mindbender. Should we invert that? The option, if we add it, is likely going to become a semi-permanent fixture going forward, even if we choose to refactor. So it's best if we add CSS classes we expect to have in the future as well.

So in principle, 👍 👍 on this moving forward, but I'd love a good code review, CC: @WordPress/gutenberg-core

Thanks for the work Andrew!

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.

Thanks for working on this! The code looks good, and testing shows everything working as expected ✅

A couple of thoughts for future improvements:

  • If we didn't enforce two equal width columns at medium-small breakpoints, but only moved to single column at small, we could do away with those !important flex basis altogether, as even fixed width columns would only need flex-grow: 1 to go 100% in single column. It might be worth checking how useful the two-column behaviour actually is and if we really need it (is it annoying users? are theme authors working around it? are there open issues about it? etc.)
  • Love the idea of using grid! I did a POC for grid layout in Columns a couple years ago, when we were having issues with the adjustable widths. Main findings:
    • in combination with custom properties it reduced the amount and specificity of the CSS dramatically;
    • the UI for setting the width of each column separately becomes awkward, because the styles are now set at the Columns block level instead; it made me wish we could either drag the columns themselves to the required size, or have a sort of layout-builder interface where we set all the widths at the parent columns level. Now that we can set fixed widths in different units for each column, that might not be super straightforward though 🤔

I'd also be keen to see some experimentation similar to @jasmussen 's codepen, though fixed width support might also be an obstacle there. Specifically, if we have a combination of fixed column widths and the expectation of a responsive layout by default, we'll probably still need some media queries.

@@ -28,66 +28,89 @@
&.are-vertically-aligned-bottom {
align-items: flex-end;
}

&:not(.is-not-stacked-on-mobile) > .wp-block-column {
Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit of moving these rules inside &:not(.is-not-stacked-on-mobile) > .wp-block-column?
These previously were on .wp-block-column directly, and it was a lot cleaner. I haven't checked, but would it not be simpler to have these in .wp-block-column like before, and then simply override these rules where necessary in &.is-not-stacked-on-mobile?
Is there a reason I'm missing for the added complexity?

Copy link
Contributor

@kjellr kjellr Jul 6, 2021

Choose a reason for hiding this comment

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

The main benefit is that using the :not() rules avoids duplicate code. There are also some !important rules here, so we'd need to override those with more !important rules. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's an awkward rule, but results in fewer lines of CSS, while preserving the existing styling / similar specificity. I couldn't quite come up with a neat way to handle it otherwise, so it was a bit of a trade-off! 😅

@ndiego
Copy link
Member

ndiego commented Jul 7, 2021

Thanks for all the work on this everyone. Very much looking forward to this! It is definitely needed. I have tested and it does the trick.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

I think we're good on this one now that we've had a couple sanity checks plus code review. As long as this passes tests, I think it's good to merge, and we can readdress/refactor in the future with grid and/or fewer :not() rules. 🙌

@kjellr kjellr merged commit 1eaad3c into WordPress:trunk Jul 7, 2021
@github-actions github-actions bot added this to the Gutenberg 11.1 milestone Jul 7, 2021
@andrewserong
Copy link
Contributor Author

Thanks for all the reviews and feedback, everyone! (And for merging this in while I was AFK, Kjell!)

Great points re: styling specificity, mobile breakpoints, and things we can continue to improve in follow-ups. Hopefully we can address some of these in the context of the work on gap support, and explorations into CSS Grid. But for now, I'm pleased we're able to get this feature in. Thanks!

sarayourfriend pushed a commit that referenced this pull request Jul 15, 2021
…ut mobile breakpoints (#31816)

* Columns block: Add stack on mobile setting

* Update selector to specify the direct child

* Fix deprecations to include the new attribute, add another set of test fixtures

* Add nowrap to is-not-stacked-on-mobile

Co-authored-by: Andrew Serong <andrewserong@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: allow Columns block non-responsive option
7 participants