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: remove custom column block styles #1133

Merged
merged 5 commits into from
May 25, 2022
Merged

Conversation

laurelfulford
Copy link
Contributor

@laurelfulford laurelfulford commented May 10, 2022

All Submissions:

Changes proposed in this Pull Request:

The Columns Block is being reworked in WP 6.0: the responsive behaviour is getting updated, and the block is moving from using margin to add space between the columns to using gap.

Originally the Columns block would break into two columns at the tablet breakpoint, and one column on mobile. This created a weird "orphaned" column situation when you had an odd number of columns, so we included some styles in the Newspack Blocks plugin to override it and make the columns number remain the same until the mobile breakpoint.

In 6.0, the Columns block will now switch to one column on tablet-sized screens, which is very close to our Newspack-specific overrides.

Here's a screenshot of the Core columns block (minus any Newspack styles) with the new breakpoint behaviour from 6.0 on the left, and the original breakpoint behaviour on the right:

columns - 6 0 and 5 9

It makes sense to remove our custom styles now since the Core behaviour will work well on our sites, and it'll save us having to make extensive updates every time the Columns block is updated going forward. This PR removes the unneeded columns styles, and switches the block to use gap instead of margin for column spacing. It also includes some 5.9-specific styles, so these changes can be pushed out prior to 6.0 without breaking anything.

This PR needs to be tested with Automattic/newspack-theme#1801, which removes some related styles from the theme as well.

Closes #1128, #1065 and Automattic/newspack-theme#840

How to test the changes in this Pull Request:

Note: To test this PR, you'll need test sites where you can run the latest WP 6.0 RC, and WP 5.9.3. You can do this by testing on two sites, or using the WordPress Beta Tester plugin to switch a single site.

  1. Start with a test site using WordPress 5.9.3.

  2. Add a series of column blocks to the editor -- I would recommend including combinations of the following:

    • Blocks with different column counts (one, two, three, four...)
    • Blocks with the 'borders' style
    • Blocks set to not wrap on mobile
    • Blocks with the columns reordered using the 'move first to second or third' block styles
    • Blocks with columns set to different widths
    • Wide and full aligned columns blocks
    • Columns blocks with background colours.

    (I created some test content that can be used as a starting point; I would recommend adding some more columns, though, in case you find things I didn't!)

  3. View the blocks on the front-end at varying screen sizes, noting how the borders look, and when the reordered blocks switch order -- all that fun stuff.

5.9 current screenshots

Desktop

image

Tablet

image

Mobile

image

Desktop

image

Tablet
image

Mobile

image

Desktop

image

  1. Apply both this PR on the block plugin, and Columns Block: remove custom column block styles newspack-theme#1801 on the theme, and run npm run build on each.
  2. Retest the above; outside of the columns breaking at the tablet breakpoint, things should function the same -- specific things to note are the columns that are "reordered" (they should be in the correct order when stacked), and the columns with borders.
  3. Upgrade your test site to 6.0, or switch to your separate 6.0 test site, and apply both this PR on the block plugin, and Columns Block: remove custom column block styles newspack-theme#1801 on the theme, and run npm run build on each.
  4. Retest the columns at different breakpoints; confirm that the behaviour is consistent with how the columns behaved in 5.9, and that you don't notice any issues with the reordering, borders, or space between columns.
  5. Test in the editor and make sure there are no obvious visual issues compared to the front-end (it may not be perfectly 1:1, but that's pretty normal -- as long as it's a good representation of how things look on the front end, it should be good.
  6. Double-check that the column block with a background is the same width as the rest of the content -- this resolves a longstanding issue in the theme:

image

  1. Test 'Newspack Subscribe Pattern 9', which uses the columns block -- though the breakpoint is a bit different (it stacks on tablet now, instead of just mobile), the vertical spacing should be the same on wrap.
Subscribe Pattern 9 - WP 5.9

Desktop:
5 9 - Screen Shot 2022-05-11 at 4 14 11 PM

Tablet:
5 9 - Screen Shot 2022-05-11 at 4 14 16 PM

Mobile:
5 9 - Screen Shot 2022-05-11 at 4 14 22 PM

Subscribe Pattern 9 - WP 6.0

Desktop:
6 0 w:changes - Screen Shot 2022-05-11 at 4 28 12 PM

Tablet:
6 0 w:changes - Screen Shot 2022-05-11 at 4 30 19 PM

Mobile:
6 0 w:changes - Screen Shot 2022-05-11 at 4 28 22 PM

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@laurelfulford laurelfulford changed the title fix: remove custom column block styles Columns Block: remove custom column block styles May 10, 2022
@laurelfulford laurelfulford marked this pull request as ready for review May 12, 2022 01:27
@laurelfulford laurelfulford requested a review from a team as a code owner May 12, 2022 01:27
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

This one is a doozy because there are SO many variations of content that need to be handled. I ran into a few issues an issue (the others were due to me not having the right Theme branch checked out 🤦 ) with the test content you provided, confirmed in both Chrome and Firefox.

The four-columns-with-border-don't-stack-on-mobile styles appear broken on mobile. This happens with both WP 5.9.3 and 6.0. Only seems to affect this particular combination of styles with more than three columns, but the borders appear off-center at mobile breakpoints:

Screen Shot 2022-05-19 at 4 44 24 PM

@laurelfulford
Copy link
Contributor Author

Thanks @dkoo! 🙏

The four-columns-with-border-don't-stack-on-mobile styles appear broken on mobile

I think this might have to do with my test content, and the fact the border styles eat up a lot of space. I tried adding red borders to the .wp-block-column blocks on my test site, and made the paragraphs semi-transparent -- it helps show that the borders are aligned correctly, but the paragraphs are overflowing the available space:

Screen Shot 2022-05-20 at 9 37 51 AM

Though it's an edge case, this isn't great 😕 I've decreased the column gap a bit for these not-stacked columns on smaller screens in a683470 -- I don't think it'll fix it if you switch to five columns, but at that point I think you could truly argue it's something you shouldn't be doing:

image

What's strange is that some of these combos—like reordering columns AND retaining borders—don't seem possible to create using the UI...

This is absolutely correct -- you can't add multiple block styles to the columns. You can manually add one of the block style classes though -- and I know we've done this in the past 😬 So I included some cases like that to make sure things will still look okay.

@laurelfulford laurelfulford requested a review from dkoo May 20, 2022 17:36
Copy link
Contributor

@dkoo dkoo 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 this might have to do with my test content, and the fact the border styles eat up a lot of space. I tried adding red borders to the .wp-block-column blocks on my test site, and made the paragraphs semi-transparent -- it helps show that the borders are aligned correctly, but the paragraphs are overflowing the available space:

I see what you mean. This is an extreme edge case, anyway, and the tweak does help this particular test content look better at small viewports.

@laurelfulford
Copy link
Contributor Author

Thanks @dkoo!

@laurelfulford laurelfulford merged commit bd79783 into master May 25, 2022
@laurelfulford laurelfulford deleted the fix/columns branch May 25, 2022 19:05
matticbot pushed a commit that referenced this pull request Jun 2, 2022
# [1.52.0-alpha.1](v1.51.0...v1.52.0-alpha.1) (2022-06-02)

### Bug Fixes

* correct donate block tab spacing in editor ([#1153](#1153)) ([c17221f](c17221f))
* disambiguate WP users vs. guest authors with same ID ([#1143](#1143)) ([d3c5920](d3c5920))
* echo closing link tags on sponsor bylines ([#1152](#1152)) ([f80893f](f80893f))
* remove custom column block styles ([#1133](#1133)) ([bd79783](bd79783))
* skipped linked images when navigating blocks by keyboard ([#1144](#1144)) ([8975787](8975787))

### Features

* add new subscribe pattern ([#1142](#1142)) ([97d632e](97d632e))
* remove support for the Aside post format ([#1139](#1139)) ([9f9cdf4](9f9cdf4))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.52.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jun 13, 2022
# [1.52.0](v1.51.0...v1.52.0) (2022-06-13)

### Bug Fixes

* correct donate block tab spacing in editor ([#1153](#1153)) ([c17221f](c17221f))
* disambiguate WP users vs. guest authors with same ID ([#1143](#1143)) ([d3c5920](d3c5920))
* echo closing link tags on sponsor bylines ([#1152](#1152)) ([f80893f](f80893f))
* remove custom column block styles ([#1133](#1133)) ([bd79783](bd79783))
* skipped linked images when navigating blocks by keyboard ([#1144](#1144)) ([8975787](8975787))

### Features

* add new subscribe pattern ([#1142](#1142)) ([97d632e](97d632e))
* remove support for the Aside post format ([#1139](#1139)) ([9f9cdf4](9f9cdf4))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Columns Block: Column spacing wrong in editor and on front-end
3 participants