Skip to content

Conversation

drdogbot7
Copy link

Description

Removes the non-working 2-column breakpoint and resolves margin-related bugs caused by it.

This is discussed extensively in #12199 and #12816, both of which attempt to fix the bugs while retaining the 2-column breakpoint.

This is an alternative approach that simplifies the code significantly and which should make things easier on theme-creators in the future.

How has this been tested?

Locally using Docker environment. Chrome, Safari, Firefox on Mac. Need to test IE11/Edge.

Types of changes

SCSS changes only. Almost exclusively removing code that didn't work properly anyway. This may cause some minor disruption in themes (including Twenty-Nineteen) that already implemented their own fixes to this block.

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.

@gziolo gziolo requested a review from jasmussen February 5, 2019 16:13
@gziolo gziolo added the [Block] Columns Affects the Columns Block label Feb 5, 2019
@gziolo gziolo requested review from kjellr and melchoyce February 5, 2019 16:14
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
@gziolo gziolo added the Needs Design Feedback Needs general design feedback. label Feb 5, 2019
@jasmussen
Copy link
Contributor

Thanks so much for keeping on this, your help fixing up the columns stuff has been tremendously valuable.

Here's a GIF:

columns

In this PR, there's 1 column until after the medium breakpoint, where the user-set amount of columns are shown.

This is certainly simpler — it simplifies the CSS to be more understandable, and it avoids us having to stack things. However in light of the arguments in #12199 (comment), and the creation of #13363 to track additional responsive enhancements — is this PR something we should land or will it be subsumed by other improvements?

@drdogbot7
Copy link
Author

However in light of the arguments in #12199 (comment), and the creation of #13363 to track additional responsive enhancements — is this PR something we should land or will it be subsumed by other improvements?

#13363 is really cool. It would give a lot more control to the end-user via the GUI. So the end user could set up columns however they want without fussing with code. If this is implemented, it will mean completely re-writing a lot of the css anyway.

#13605 (this PR) takes the opposite approach and keeps the columns block as simple as possible, and leaves it up to the theme designer to add any intermediate breakpoints.

I don't know what is better. I think it really depends on what the role of the core gutenberg blocks is.

@kjellr
Copy link
Contributor

kjellr commented Feb 6, 2019

This is certainly simpler — it simplifies the CSS to be more understandable, and it avoids us having to stack things. However in light of the arguments in #12199 (comment), and the creation of #13363 to track additional responsive enhancements — is this PR something we should land or will it be subsumed by other improvements?

I will add that one of the intentions of #13363 is to continue putting the onus on theme (or core) developers to build in the best-case recommended behavior for these breakpoints. If the end user chooses to override that, we'd drop whatever smart responsive behavior we've built in, and let them manually adjust behavior across a few key breakpoints.

So from our perspective here, I think the main question is: will dropping the middle-breakpoint for the columns block result in the smartest behavior for this block? I'd lean towards saying no... after all, this is a "Columns" block. I'd expect to see more than one column on most devices.

@drdogbot7
Copy link
Author

I'm going to go ahead and close this.

@drdogbot7 drdogbot7 closed this Feb 6, 2019
@jasmussen
Copy link
Contributor

Can I just say thank you for all the work you've put into this? Please consider adding yourself to https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTORS.md, I will approve the PR in a heartbeat. Or, if you'll permit me to add you, you totally should be there.

@kjellr
Copy link
Contributor

kjellr commented Feb 6, 2019

Can I just say thank you for all the work you've put into this?

+1. Thank you, @drdogbot7! You've really helped sort out some confusing details around this.

@drdogbot7
Copy link
Author

Thanks guys! I'll be excited to see where this goes. Glad to be part of the discussion!

@jasmussen
Copy link
Contributor

Created #13724.

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 Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants