Try/refactor

[shaunandrews]

Ok, so. I ended up deleting a bunch of CSS, reorganizing, and rewriting/refactoring just about... well... everything.

I know this is a big PR, and that it would be better if I broke up up into dozens (or more) smaller PRs — but I'm not sure its worth the extra effort. What I hope this PR does is create a solid foundation for us all to continue to improve. I'm sure I've done many things the "wrong way," or that there might be regressions from trunk, but overall I feel these changes are a net positive to the state of the theme.

[ryelle]

🥇 💯 for going through all this! I agree that this feels like a much better foundation.

I spot checked a handful of files and clicked around a bunch of pages. There are a few things that got unstyled (like pagination), and there are some accessibility regressions (people of wp), but IMO if we're going to use this structure it would be better to merge it & iterate from there.

The files need a linting, run yarn workspace wporg-news-2021-theme lint:css --fix for automated fixes, and a few things that need to be fixed manually.

The rest of my comments can either be fixed here or later, your call.

I also noticed a scrollbar on the front page between 782px-1385px:

The subscription button got unstyled (this one's hard since you can't see it locally :/ )

I would also wait for a 👍🏻 from @iandunn, since he set the project up and there might be context I don't have for some of the previous code.

[ryelle]

Suggested change

$category_slugs = wp_list_pluck( get_the_category( $id ), 'slug' );

You can get rid of this since you're not checking for it on the next line.

[shaunandrews]

Ah, I'm glad you noticed. I'm not entirely sure if this function is too generic — I just copy/pasted it from the function above.

[ryelle]

yeah, this could probably be re-refactored back into one update_the_title function that does both things.

[ryelle]

This is putting a margin on subsequent columns in a columns block:

[apeatling]

Nice work on this, it's a huge effort!

I've gone over it and overall the design is looking more put together, I'm not noticing anything hugely out of place. There are some small things, but I think they are on your radar.

In terms of the organization of the files, it does feel to me more intuitive. It's more organized around the actual site so it's easier to know where to go to find something. In my opinion for a bigger application the previous structure makes more sense -- but for a less complex site I think this is more manageable.

In my opinion this could merge and improvements continue from there. I haven't been super involved here, so just putting in my thoughts having looked at this for the past week!

[iandunn]

There's some good stuff in here, but I've spent more than half the day trying to give it a thorough review, and I'm only done w/ 40 of the 73 files. Even with those, it often wasn't practical to be thorough, since it's too time intensive to map all of the connections, work out exactly what changed, and try to infer why when it wasn't obvious.

I trust Kelly's review, and I feel like it'd be better for me to focus on the launch tasks than to finish reviewing this.

[iandunn]

Suggested change

	// Blocks, by default, have a top margin. This is annoying.
	// Blocks, by default, have a top margin, but we usually only want that in post content. When something outside post content needs a margin, it usually needs to be custom anyway, rather than exactly matching `--wp--style--block-gap`.

I think this describes what you were implying, but feel free to make it more accurate if I misunderstood. I agree it's annoying, I just want us to remember why when we come back to this in 6 months.

[shaunandrews]

Rather than try to rebase and merge this branch, I ended up creating a handful of PR's that (mostly) cover the changes here: #214, #215, #216, #217, #218, #219, #220,#221, #222