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

"Releases" Template #246

Merged
merged 25 commits into from Feb 14, 2022
Merged

"Releases" Template #246

merged 25 commits into from Feb 14, 2022

Conversation

shaunandrews
Copy link
Collaborator

@shaunandrews shaunandrews commented Jan 25, 2022

image

This PR updates the Releases category template to more closely match the design. It also includes a nifty, new block for displaying a release version number.

@iandunn
Copy link
Member

iandunn commented Jan 25, 2022

What do you think about a custom block that pulls the x.y out of the title w/ a regex? #38 has the regex worked out already. That seems like less technical debt to me, because it's more straightforward and future-proof.

We could also use it on the homepage to match the design closer, and remove this (see #119):

/**
* Remove "WordPress" from the release post title.
*
* @param string $title The post title.
* @param int $id The post ID.
* @return string Filtered post title.
*/
function update_the_title( $title, $id ) {
// Remove "WordPress" from the post title in the Latest Release section on the front page.
$category_slugs = wp_list_pluck( get_the_category( $id ), 'slug' );
if ( is_front_page() && in_array( 'releases', $category_slugs ) ) {
return str_replace( 'WordPress', '', $title );
}
return $title;
}

Something similar would work for the date in the Events template.

Related: #75

@tellyworth
Copy link
Contributor

I pushed a commit to add a custom block. Seems to work ok. @shaunandrews I've only lightly touched your CSS, the rest of it is additions.

To get it to work you'll probably need to:

composer update && wp-env start --update

@iandunn have I added the block plugin in a sane way? I copied the gitignore handling of the theme folder basically.

.gitignore Outdated
@@ -30,5 +31,9 @@ README-svn.txt
!/source/wp-content/themes
!/source/wp-content/themes/wporg-news-2021

# And this plugin.
!/source/wp-content/plugins
!/source/wp-content/plugins/wporg-post-title
Copy link
Member

Choose a reason for hiding this comment

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

🤔 , I'd lean towards putting the code in the blocks/ folder in the mu-plugins repo, so that all sites can use it. I'd guess that future redesigns will have a similar concept, with part of the post title being used as a design element.

I don't feel strongly, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think other sites will need this exact functionality, but if we leave it here that's a third folder we would need to sync into SVN.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I change it to a more specific block like post-title-version, would any other sites have a use for it? It's a pretty obscure use case. Agree that mu-plugins could make sense for a more generally reusable block though.

Copy link
Member

Choose a reason for hiding this comment

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

Kelly has a good point. I could even seen this being put in functions.php since it's really just a glorified custom template tag. I wouldn't do that for normal blocks, but this is just a workaround for FSE limitations. I don't feel strongly though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with leaving it in the theme (maybe a blocks folder, in case we need any other workarounds). If we're still going the parent-theme route, they'll be available to any child themes too. I do think we should use a block.json + set it up in JS too, so that it won't break in the site editor (see WordPress/wporg-mu-plugins#26) when we get there.

*
* @return string Returns the filtered post title for the current post wrapped inside "h1" tags.
*/
function render_block_wporg_post_title( $attributes, $content, $block ) {
Copy link
Member

Choose a reason for hiding this comment

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

I was imagining something much more simple, that would only be used for this purpose, since core/post-title can be used if they want the full title.

register_block_type(
	'wporg/post-title-version',
	array(
		'title'           => 'Post Title Version',
		'render_callback' => function() {
			$full_title = get_the_title();
			preg_match( '/WordPress (\d+[.]\d+)/', $full_title, $matches );
			
			return $matches[1] ?? ''; 
		}
	)
);

That's probably missing a few things, but communicates the jist of it. I don't think we need block.json etc for a small workaround like this. In my mind this is just a temporary thing until Gutenberg solves WordPress/gutenberg#32939.

Comment on lines 4 to 6
"title": "WordPress.org Post Title",
"category": "theme",
"description": "Extends the core Post Title block with additional attributes needed for WordPress.org themes.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside from the releaseVersion toggle, how is this different from the Post Title block? Only asking because when I saw "custom block for the version" I thought of making a simple block that only outputs the release version, with no other attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I like that idea. I guess I was preempting other hypothetical uses for the same block, but I don't think we have any other actual use cases right now.

@tellyworth
Copy link
Contributor

I changed the block to wporg/release-version. It just outputs the version number and a wrapper tag, rather than duplicating post title stuff.

It feels like overkill. The alternative to adding the block in a plugin is to register a block from the theme itself, which I think(?) is generally discouraged.

@ryelle
Copy link
Contributor

ryelle commented Jan 27, 2022

The alternative to adding the block in a plugin is to register a block from the theme itself, which I think(?) is generally discouraged.

That's true, but that actually was what I was suggesting 😅 I guess it depends on whether you think we'll need this on sites that don't use this theme (itself or as a parent), or if we might want it in the future, whenever this theme is eventually retired. My opinion is that it's decorative in a way that's tied to the theme, so I think keeping it in the theme is fine.

@shaunandrews

This comment has been minimized.

Comment on lines 40 to 41
// Do we also want x.y.z?
if ( preg_match( '/WordPress (\d+[.]\d+)/', $title, $matches ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A regex that will work for getting 3rd-level versions is (\d{0,3}(\.\d{1,3})+) (from here)

Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 It seems important to get .z in there

@shaunandrews shaunandrews changed the title First pass at using a duplicate post-title block to accomplish the version numbers. "Releases" Template Jan 31, 2022
@shaunandrews
Copy link
Collaborator Author

Right now the "5.8" version number is duplicated many times, since each of the latest posts (from the import) is for 5.8 release candidates. Would it make sense to update these version numbers to include "RCx" to help reduce the repetition? This might require some design thinking, as I think "5.8 RC2" might be too long to fit within the available space.

@beafialho
Copy link
Collaborator

beafialho commented Feb 1, 2022

Not sure why I'm not seeing the numbers on the left?

Captura de ecrã 2022-02-01, às 11 35 10

@tellyworth
Copy link
Contributor

Not sure why I'm not seeing the numbers on the left?

Did you run this? -

composer update && wp-env start --update

@tellyworth
Copy link
Contributor

tellyworth commented Feb 2, 2022

I updated the PR to:

  • Remove the separate plugin, and register the block from functions.php
  • Handle x.y.z and RC n

Do we also want it to understand Beta n? That text is probably too long to fit.

Screenshot (zoomed out to fit more examples):
Screen Shot 2022-02-02 at 12 20 47 pm

Make sure you run composer update && wp-env start --update to test it locally.

@beafialho
Copy link
Collaborator

Did you run this? -

I hadn't, that worked. Thanks!

Do we also want it to understand Beta n? That text is probably too long to fit.

Let's keep the numbers only as they fit just fine. I like the "RC" alternative.

Overall, this looks great! I have some comments:

Desktop

  • The height of each post should ideally be 174px instead of the current 232px:
Currently Mockups
Captura de ecrã 2022-02-02, às 14 38 09 Captura de ecrã 2022-02-02, às 14 40 13
  • Increase spacing down at the bottom of the page (should look the same as on top of posts navigation):
Currently Intended
Captura de ecrã 2022-02-02, às 14 30 23 Captura de ecrã 2022-02-02, às 14 31 06
  • Hover effect missing in Read Post (underline should disappear on hover):
hover.mp4

Mobile

  • Let's remove the arrow on mobile:

Captura de ecrã 2022-02-02, às 14 34 37

  • Increase spacing between posts (top and bottom spacing should be identical)
Currently Intended
Captura de ecrã 2022-02-02, às 14 34 50 Captura de ecrã 2022-02-02, às 14 36 31
  • Posts navigation off the page:
weird.mp4

shaunandrews and others added 5 commits February 10, 2022 13:45
Sorry if this steamrolls WIP @shaunandrews!

I added a custom post title block with a `releaseVersion` attribute to output just the version number. Feels like overkill but anything else I came up with was super hacky.

It seems to play ok with your CSS, I bodged it a bit to work. Not sure if other styling is needed.
@ryelle
Copy link
Contributor

ryelle commented Feb 11, 2022

@coreymckrill I made a few changes to the block markup, since we don't need to have this in a heading we can avoid the aria workarounds. The content should still be available to screen reader users (just not as a heading). Feel free to undo anything you don't like :)

I think all of @beafialho's feedback is still relevant - I removed the arrow on small screens while restructuring some of the CSS, but things like the post height & wonky pagination on small screens are still happening.

@coreymckrill
Copy link
Contributor

@beafialho @ryelle Sorry I missed that some of the feedback from earlier hadn't been addressed yet. It has been now, would love to get your review. One thing to note is that I also moved the version numbers for the not-latest release posts down to the bottom edge of the rows on mobile to better match the Figma design.

@beafialho
Copy link
Collaborator

Thank you @coreymckrill this looks great!

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Overall this is working great, I just caught a few things while looking through the code.

I also noticed the pagination is missing space between 600-780px:

Screen Shot 2022-02-14 at 11 03 49

@coreymckrill coreymckrill merged commit e087a05 into trunk Feb 14, 2022
@coreymckrill coreymckrill deleted the try/releases-category branch February 14, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants