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

remove broken 2-column breakpoint from columns block #13605

Closed

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 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