Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

Try: Add comments. #156

Merged
merged 8 commits into from
Sep 1, 2024
Merged

Try: Add comments. #156

merged 8 commits into from
Sep 1, 2024

Conversation

juanfra
Copy link
Member

@juanfra juanfra commented Aug 29, 2024

Description

Fixes #115

  • Adding comments configuration to theme.json
  • Adding comments hidden pattern.
  • Including comments in the single template.

🗒️ Notes

  1. I set some 5px values now that we no longer have that as a preset.
  2. I had to set some custom CSS to override the default styles of the form inputs.

Screenshots

Screen.Recording.2024-08-29.at.17.28.54.mov

Testing Instructions

  1. Create a post, be sure that the template is using the comments.
  2. Leave as many comments as you like.
  3. Confirm that the implementation looks like the designs

Copy link

github-actions bot commented Aug 29, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: juanfra <juanfra@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: beafialho <beafialho@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@juanfra juanfra self-assigned this Aug 29, 2024
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

PR looks good to me, works well. I left a few comments. Short of approving this, as I'm feeling out how I can best contribute to this project, I'll defer to Carolina and/or Bea on details. Though it seems like we could merge and iterate.

@@ -0,0 +1,51 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish twe didn't have to store templates in the patterns folder in order for them to be translatable. I guess there's no real way around that?

Copy link
Contributor

@carolinan carolinan Aug 30, 2024

Choose a reason for hiding this comment

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

No. I don't know if there is an open issue that covers it better than this one:
WordPress/gutenberg#20966

The project is closer to solving it now that there are block bindings.
In theory, block bindings could already be used, since you can use custom PHP functions which could include the translations.... but I don't think this theme should experiment with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this particular part of the theme is not a template or a part, it is intended to be a pattern so that it can be inserted anywhere, and it would be PHP file even if it had no translatable strings.

@@ -931,6 +931,11 @@
"lineHeight": "1.4"
},
"blocks": {
"core/avatar": {
"border": {
"radius": "100px"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to create some radius presets, of which one preset could be "round", and just set to 9999px?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a request in the Gutenberg repository to add this to the editor.
Let's check how far that has come, but I think it needs dev:
WordPress/gutenberg#64041

Copy link
Contributor

Choose a reason for hiding this comment

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

I will add the issue to the list of issues.

theme.json Outdated
"typography": {
"fontSize": "var:preset|font-size|small"
}
},
"core/post-comments-form": {
"css": "& textarea, input:not([type=submit]){border-radius:.25rem; border-color: var(--wp--preset--color--opacity-20) !important;} & input[type=checkbox]{margin:0 .2rem 0 0 !important;} & label {font-size: var(--wp--preset--font-size--x-small); }",
Copy link
Contributor

Choose a reason for hiding this comment

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

Some finnicky details here. But notably the input radius It'd be nice to again add to a preset scale, so they're consistent across.

@carolinan
Copy link
Contributor

carolinan commented Aug 30, 2024

Because the page templates are designed to not have comment areas by default, I think it would be good to not make this a hidden pattern. That way the user can add a new comment area that looks as good.

@beafialho
Copy link
Contributor

Thank you, this is looking good to me!

Rename the pattern file and remove inserter: no.
Fix the block validation error with the comments area heading.
Update the heading level hierarchy and the font sizes of the comments  area heading and the comments title.
Replaced x-small with small
@carolinan carolinan merged commit e1e395c into trunk Sep 1, 2024
4 checks passed
@juanfra juanfra deleted the try/add-comments branch September 2, 2024 12:33
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.

Add comments area to posts
4 participants