Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

Update single and page, add comments part #55

Merged
merged 13 commits into from
Aug 23, 2022
Merged

Update single and page, add comments part #55

merged 13 commits into from
Aug 23, 2022

Conversation

carolinan
Copy link
Collaborator

@carolinan carolinan commented Aug 16, 2022

Updates single post and page, adds a template part for comments.
Closes #45

@carolinan
Copy link
Collaborator Author

Decision needed:

  1. There is no way to separate the post terms block in two lines, one with the text "Tags:" and one with the actual tags, without adding CSS.
  2. But when using a paragraph, the text is always visible even when there are no tags.

@carolinan carolinan marked this pull request as ready for review August 16, 2022 07:19
@carolinan carolinan changed the title Update single, add comments part Update single and page, add comments part Aug 16, 2022
@beafialho
Copy link
Collaborator

I managed to achieve this with the markup below, what do you think?

Captura de ecrã 2022-08-16, às 10 31 21

<!-- wp:group {"layout":{"inherit":true}} -->
<div class="wp-block-group"><!-- wp:separator {"align":"wide","className":"is-style-default"} -->
<hr class="wp-block-separator alignwide has-alpha-channel-opacity is-style-default"/>
<!-- /wp:separator -->

<!-- wp:group {"align":"wide","layout":{"type":"flex","justifyContent":"space-between","verticalAlignment":"top"}} -->
<div class="wp-block-group alignwide"><!-- wp:group -->
<div class="wp-block-group"><!-- wp:group {"style":{"spacing":{"blockGap":"10px"}},"layout":{"type":"flex","orientation":"vertical"}} -->
<div class="wp-block-group"><!-- wp:group {"layout":{"type":"flex","flexWrap":"nowrap"}} -->
<div class="wp-block-group"><!-- wp:post-date {"format":"F j, Y","fontSize":"small"} /-->

<!-- wp:post-terms {"term":"category","style":{"typography":{"fontStyle":"normal","fontWeight":"400"}},"fontSize":"small"} /--></div>
<!-- /wp:group -->

<!-- wp:post-author {"showAvatar":false,"byline":"","fontSize":"small"} /--></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:group {"style":{"spacing":{"blockGap":"10px"}},"layout":{"type":"flex","orientation":"vertical"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"fontSize":"small"} -->
<p class="has-small-font-size">Tags:</p>
<!-- /wp:paragraph -->

<!-- wp:post-terms {"term":"post_tag","style":{"typography":{"fontStyle":"normal","fontWeight":"400"}},"fontSize":"small"} /--></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:spacer -->
<div style="height:100px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:post-comments /--></div>
<!-- /wp:group -->

@pbking
Copy link
Collaborator

pbking commented Aug 16, 2022

I like when using the prefix attribute there's no label if there's no tags.

I don't like that when using the 'prefix' there's no way for a user to change that label (yet).

I'm impartial if the tags start on the next line or not. I definitely don't think we should be adding CSS to achieve that.

My two cents is that using a paragraph is the best solution. Though I suppose that would require making that a pattern so that it can be localized.

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 16, 2022

I managed to achieve this with the markup below, what do you think?

I did not understand, did you mean that it should use groups, not the columns?

@carolinan
Copy link
Collaborator Author

I don't like that when using the 'prefix' there's no way for a user to change that label (yet).

If the theme will have a minimum WP version of 6.1, the users can change the label to anything.
The feature is merged into Gutenberg, please see WordPress/gutenberg#40559
WordPress/gutenberg#42418

This is the problem that I mentioned on Slack, the block is bugged / needs testing, I was not able to get the prefix to work.

@beafialho
Copy link
Collaborator

I did not understand, did you mean that it should use groups, not the columns?

I managed to separate the tags in two lines, one with the text "Tags:" and one with the actual tags, without adding CSS, by using a stack block.

@carolinan
Copy link
Collaborator Author

I did not understand, did you mean that it should use groups, not the columns?

I managed to separate the tags in two lines, one with the text "Tags:" and one with the actual tags, without adding CSS, by using a stack block.

Yes, the issue was about not being able to do that with the post terms block only.

@beafialho
Copy link
Collaborator

Yes, the issue was about not being able to do that with the post terms block only.

Oh, in that case ignore my comment 🥲

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 17, 2022

@pbking How can we turn this sentence "Posted date in category" into a translatable string?
If we don't include the block markup in the translation function and printf(), the date format can not be localized, and if we do include the block markup, the markup feels dangerously close to breaking because of potential typos.
In addition, the block spacing is used to fake spaces between these blocks, making it more difficult to translate.

@carolinan
Copy link
Collaborator Author

Yes, the issue was about not being able to do that with the post terms block only.

Oh, in that case ignore my comment 🥲

We should still test! Because I believe there are some differences between how columns and the row block look on small screens.

This was referenced Aug 17, 2022
@mikachan mikachan linked an issue Aug 17, 2022 that may be closed by this pull request
@mikachan
Copy link
Member

As mentioned in #58, I think we should remove the single-no-separators template. This PR would be a nice place to do that, but happy for it to happen separately.

@carolinan
Copy link
Collaborator Author

  • With a columns block, the tags do not align correctly to the right.
  • The stacks need a lower vertical block spacing

500px width:
localhost_10003_2012_01_07_template-sticky_

800px width:
localhost_10003_2012_01_07_template-sticky_ (2)

1024px width:
localhost_10003_2012_01_07_template-sticky_ (1)

@carolinan
Copy link
Collaborator Author

With the group blocks from comment 2.

500px width:
localhost_10003_2012_01_07_template-sticky_ (3)

800px width:
localhost_10003_2012_01_07_template-sticky_ (4)

@carolinan
Copy link
Collaborator Author

When there are many categories:

Using columns:
localhost_10003_2009_07_02_edge-case-many-categories_ (1)

Using groups:
localhost_10003_2009_07_02_edge-case-many-categories_

@mikachan mikachan linked an issue Aug 18, 2022 that may be closed by this pull request
@mikachan
Copy link
Member

The columns are generally looking better to me (I think).

With a columns block, the tags do not align correctly to the right.

What about trying an empty column in the middle of the two existing ones? So a 3 column layout with an empty column in the middle. This pushes the third column further to the right, but perhaps makes the column widths too small.

Here's an empty column for easy copy/pasting:

<!-- wp:column -->
<div class="wp-block-column"></div>
<!-- /wp:column -->

@carolinan
Copy link
Collaborator Author

carolinan commented Aug 22, 2022

I have not been able to find a solution for making the "posted (date) in (category)" translatable. If there is any language where the order of the words would need to be changed, they would need to move the whole block, not just the text but the whole markup.

Increase the space between the columns, decrease the space inside the first column.
<hr class="wp-block-separator alignwide has-css-opacity is-style-wide"/>
<!-- /wp:separator -->

<!-- wp:columns {"align":"wide","style":{"spacing":{"margin":{"top":"var(--wp--preset--spacing--70)"},"blockGap":"var(--wp--preset--spacing--70)"}},"fontSize":"small"} -->
Copy link
Collaborator Author

@carolinan carolinan Aug 22, 2022

Choose a reason for hiding this comment

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

The paragraph block, post date and terms have different font sizes, adding the font size to the columns evens this out.
The caveat is that this can't be changed with style variations.


<!-- wp:columns {"align":"wide","style":{"spacing":{"margin":{"top":"var(--wp--preset--spacing--70)"},"blockGap":"var(--wp--preset--spacing--70)"}},"fontSize":"small"} -->
<div class="wp-block-columns alignwide has-small-font-size" style="margin-top:var(--wp--preset--spacing--70)">
<!-- wp:column {"style":{"spacing":{"blockGap":"0px"}}} -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

blockGap:0 here reduces the vertical spacing between the rows of text.

<hr class="wp-block-separator alignwide has-css-opacity is-style-wide"/>
<!-- /wp:separator -->

<!-- wp:columns {"align":"wide","style":{"spacing":{"margin":{"top":"var(--wp--preset--spacing--70)"},"blockGap":"var(--wp--preset--spacing--70)"}},"fontSize":"small"} -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"blockGap":"var(--wp--preset--spacing--70)"

Using the spacing preset inside blockGap does not seem to work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrewserong I am trying to use a spacing preset for a blockGap in a columns block, but in the block markup in the HTML file, not in theme.json. I can't get it to work. Is this known? I could not find an issue for it, WordPress/gutenberg#43296 is the closest.
The gap CSS is missing from the columns .wp-container-id in the editor.
If I change it to a different value like 2rem, the CSS is added correctly.
(I am testing with Gutenberg trunk)

Copy link

@andrewserong andrewserong Aug 23, 2022

Choose a reason for hiding this comment

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

Hi @carolinan, thanks for the ping! It isn't quite working just yet, that PR unfortunately missed a couple of things to get it working properly. I'm currently working on a fix (and opt-in to the spacing controls) over in WordPress/gutenberg#43466, which I hope to get in this week (it isn't yet ready for review, there's still a few things to tweak)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you :) Then I think it can be kept in the template.

Choose a reason for hiding this comment

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

One thing to watch out for when using spacing presets (also for padding and margin) is that the editor will serialize the values to post content using the form var:preset|spacing|30 instead of using the var() function directly, in the JSON part of the block markup. For example, this is how the Group block with padding and blockGap values currently looks in my local environment when added via the UI:

<!-- wp:group {"layout":{"type":"constrained"},"style":{"spacing":{"padding":{"top":"var:preset|spacing|40","right":"var:preset|spacing|40","bottom":"var:preset|spacing|40","left":"var:preset|spacing|40"},"blockGap":"var:preset|spacing|30"}}} -->
<div class="wp-block-group" style="padding-top:var(--wp--preset--spacing--40);padding-right:var(--wp--preset--spacing--40);padding-bottom:var(--wp--preset--spacing--40);padding-left:var(--wp--preset--spacing--40)"><!-- wp:paragraph -->
<p>A paragraph in a group block</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

In that example, note that the JSON section uses the form var:preset|spacing|40, whereas the inline style uses the var() function in the HTML markup. Because blockGap is rendered on the server, it doesn't appear in the inline style.

Hope that helps!

Copy link

@andrewserong andrewserong Aug 26, 2022

Choose a reason for hiding this comment

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

@carolinan just a heads-up that WordPress/gutenberg#43466 has now been merged, so the block spacing UI should now be working correctly with presets. Hopefully that'll make it a little easier building out the parts of the theme that depend on block spacing presets. I think you'll probably wind up needing to update the markup to use the var:preset|spacing|40 syntax in the block comments as the server rendering might not like the var() style, but I haven't manually tested it. Please let me know know (or add any issues) if you run into any issues with the UI changes as there were a few edge cases that needed to be handled in #43466 to get everything working correctly 🙂

@beafialho
Copy link
Collaborator

beafialho commented Aug 22, 2022

I'm seeing a comments section in the Single Page template, but according to Figma, only the Single Post template should have a comments section. Do you think the Page should have comments as well?

Captura de ecrã 2022-08-22, às 16 24 48

@carolinan
Copy link
Collaborator Author

I think they have to have it, since you can turn comments on and off per page under the settings?

Personally, I would disable comments site wide and not use it on the template. But I think since it is a default theme, it might be too much to ask of users to edit the template and add the comments query? 🤔

@beafialho
Copy link
Collaborator

Personally, I would disable comments site wide and not use it on the template. But I think since it is a default theme, it might be too much to ask of users to edit the template and add the comments query?

I see what you mean, let's leave it there then.

@carolinan
Copy link
Collaborator Author

Can we merge this, test with more eyes on it, and iterate?

@mikachan
Copy link
Member

Yes, I was thinking similar earlier, sounds good!

@carolinan carolinan merged commit 17f396f into trunk Aug 23, 2022
@carolinan carolinan deleted the update/single branch August 23, 2022 13:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"No separators" templates Single + Page templates: match Figma Use the new comments blocks
5 participants