Skip to content

Comments

feat: remove newspack-grid, newspack-grid-small, newspack-grid-sidebar#417

Open
thomasguillot wants to merge 5 commits intotrunkfrom
update/newspack-grid-alternative
Open

feat: remove newspack-grid, newspack-grid-small, newspack-grid-sidebar#417
thomasguillot wants to merge 5 commits intotrunkfrom
update/newspack-grid-alternative

Conversation

@thomasguillot
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

This is a big one, so hopefully nothing’s broken.

This PR simplifies how columns are handled. I’ve removed the custom classes because it’s unlikely publishers will think to add them to their blocks, and they could easily be missed during migration. Instead, I’m targeting specific layouts directly, 25/50/25, 15/70/15, 33/66, and 66/33, and applying the grid to those.

The sidebar has been removed and replaced with the new grid. The same applies to newspack-grid-small, which felt redundant since the new grid covers those cases.

I’ve also removed the column patterns. Their main purpose was to include the newspack-grid class by default, but since that class is no longer used, keeping the patterns did not add much value.

Template parts have been checked to ensure everything still behaves as expected.

How to test the changes in this Pull Request:

  1. On a page, add a bunch of columns, with newspack-grid (25/50/25, 15/70/15, 33/66, and 66/33)
  2. On another page, add the same columns but without newspack-grid
  3. Check and compare both pages
  4. Switch to this branch
  5. Refresh the page that didn't have the newspack-grid –– it should now look like the page you haven't refresh
  6. Now refresh the other page and it should look exactly the same
  7. Check various parts of your site if you notice anything broken
  8. Check archive and search pages

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?

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.

1 participant