Curator: revise single template

[PeterNdomano]

…issues #5931

Changes proposed in this Pull Request:

Edited curator/templates/single.html to meet the design suggested in #5931 for single post view. Made the template file completely compatible with FSE.

Related issue(s):

#5931

[jffng]

Thank you for this @PeterNdomano ! Very cool, also nice to see the new post comments blocks in action.

I left some initial comments.

[jffng]

The width of the separator here should be 1px, and it should also extend full width (touching the edges of the screen).

[jffng]

We'll need to escape all strings, including these labels as well as line 66 ("Comments"). Also uppercase should be done via text transform / CSS.

To do this, maybe we should create a "hidden" pattern for the comments, like the 404 pattern: https://github.com/Automattic/themes/blob/trunk/curator/patterns/404.php#L5

[jffng]

Let's get rid of the theme slug here, so the templates and parts always resolve from the currently active theme:

Suggested change

	<!-- wp:template-part {"slug":"header","theme":"curator","tagName":"header"} /-->
	<!-- wp:template-part {"slug":"header","tagName":"header"} /-->

Same for the footer part.

[jffng]

There's quite a bit of space between the parenthesis and comments count here — any way we can reduce that?

[jffng]

The font family should be Work Sans for the pagination:

[PeterNdomano]

Thanks @jffng for your review. All the requested changes were covered in the commits but I would like to point out these 2 things:

1) I removed prev and next arrows in post navigation for mobile view to prevent the phrase "PREVIOUS POST" from wrapping over 2 lines. It's just my suggestion for mobile view, if it ain't right we can bring them back!.. Consider the images below.

on mobile

on desktop

2) I tried to detect if the post has a featured image inside the pattern curator/post-hero so that I can load specific cover block in order to match the targeted design, Consider images below.

without featured image

with featured image

So, to accomplish that I tried using the method below but it doesn't seem to work, would you please guide me on that.

<?php if ( has_post_thumbnail() ): ?>
<!-- wp:cover {"useFeaturedImage":true,"dimRatio":50,"overlayColor":"foreground","minHeight":100,"minHeightUnit":"vh"} -->
<!-- /wp:cover -->
<?php else: ?>
<!-- wp:cover {"overlayColor":"background","minHeight":50,"minHeightUnit":"vh","isDark":false} -->
<!-- /wp:cover -->
<?php endif; ?>

Thanks once more @jffng .

[madhusudhand]

Hi @PeterNdomano, Thanks for splitting the page into multiple patterns.
I posted few comments inline.

[PeterNdomano]

Thank you guys @madhusudhand @jffng
I'll work on the changes ASAP

[jffng]

@PeterNdomano thanks for your work here, I made a number of changes to simplify this one a bit and work within the parameters of what's possible right now.

@beafialho I think this is in a decent spot to review. Here are a few scenarios to test the Single Post template:

• With and without a featured image
• Post containing wide and full width content
• Adding some comments

I do think be useful to review and hopefully merge #5980 first, so that we can get a better sense of this template in context.

One question — is the meta supposed to display tags, categories, both, or just whether the post is featured or not?

There are a couple of issues that I think we should address in follow ups before this PR gets too big:

• The previous / next link's color and font family depend on this Gutenberg PR: Post Navigation Link: add design controls (color, text decoration, and font family) WordPress/gutenberg#41378
• Hover/active states to the Previous / Next links, as well as the post terms in the featured image area, perhaps this should be addressed with Curator: update link and button styles #6028
• I've removed the arrows from these links for now since it's not clear to me how these should be implemented, and would like to address this in a follow up PR and / or Gutenberg issue / PR.

[PeterNdomano]

Thanks @jffng I noticed that this PR still depends on some other unmerged PRs like this one on post meta and date as mentioned by @madhusudhand in the previous review. So I thought I had to wait for those PRs to get merged first, that's why I didn't commit yet after your last reviews. However, I hope review & clarification from @beafialho will help us to keep going with this PR.

[beafialho]

Thank you all for your work. @jffng I'll review the Single Post template and give feedback.

One question — is the meta supposed to display tags, categories, both, or just whether the post is featured or not?

In the post template, just categories and post date.

[beafialho]

I tried some of those options you mentioned:

• This is what a post with a long title looks like, quite tight. I tried adding 120px padding above and below (not on the sides) and it looks better.

Currently	With top and bottom padding

• The post category is underlined by default. It shouldn't be, it should be underlined on hover.

• Comments look good in general! My only comment is: can the Reply and Edit buttons use a smaller size? They shouldn't be as highlighted as the text.

[madhusudhand]

@PeterNdomano The design of the category, date pattern changed to different style in archive pages. You no longer need to depend on the other PR. Please style them with above design.

[PeterNdomano]

Thanks @beafialho for your review.

@jffng I added 120px padding on top and bottom inside the inner cover and now it looks fine. I tried to modify fontSize using {"fontSize":"medium"} inside comment-content block and {"fontSize":"small"} inside both reply and edit links in order to differentiate the sizes as suggested, but unfortunately they don't seem to work. Perhaps it's an issue with these comment blocks and maybe we should use raw CSS.

Regarding the underlining of post-category before hover I hope @jffng will have a better solution coz last time I tried to do it with raw CSS inside styles.css file

[PeterNdomano]

@PeterNdomano The design of the category, date pattern changed to different style in archive pages. You no longer need to depend on the other PR. Please style them with above design.

Thanks @madhusudhand

[jffng]

Thanks everyone!

I tried a long title and the top and bottom padding look better, but maybe we should add some to the sides as well since you can end up in situations like the following depending on the word length?

The post category is underlined by default. It shouldn't be, it should be underlined on hover.

Yeah, I was hoping we can address that as a part of #6028, since that one is handling link styles and we will want to be concise with those rules.

I tried to modify fontSize using {"fontSize":"medium"} inside comment-content block and {"fontSize":"small"}

@PeterNdomano these changes appear seem to be working for me!

[PeterNdomano]

but maybe we should add some to the sides as well since you can end up in situations like the following depending on the word length?

Yeah, that's right. I just added 20px left and right padding. I chose 20px after some number of tests to make sure it displays well on both mobile and desktop. What do you think about it @jffng

these changes appear seem to be working for me!

Ohhh...! I think it was an issue with caching on my end.

[jffng]

I just added 20px left and right padding. I chose 20px after some number of tests to make sure it displays well on both mobile and desktop. What do you think about it

Let's use the same outer spacing value as the rest of the site's padding here. I updated that in d6529ca

I've addressed the remaining feedback from @beafialho and rebased this one, I think it's in a good spot to bring in. There is one dependency on a GB PR ( WordPress/gutenberg#41378 ) but I don't think that should hold this up. Thanks @PeterNdomano @madhusudhand @beafialho!