Question from #3111409: Add new Olivero frontend theme to Drupal 9.1 core as beta

Right now comments are hard-coded to appear at the bottom of the node:

core/themes/olivero/templates/content/node.html.twig

  <div{{ content_attributes.addClass('node__content', layout) }}>
    {# Comments not part of content, so they won't inherit .text-content styles. #}
    {{ content|without('comment') }}
  </div>
  {% if content.comment %}
    <div id="comments" class="{{ layout }}">
      {{ content.comment }}
    </div>
  {% endif %}

It's not 100% clear why this is the case. Does that code comment mean that comments shouldn't inherit .text-content styles?
At the very least, this ensures that comments have a wrapper with the class of the layout, but there's other ways to do that.

But via Field UI you can position the comments (and any other field) into any order. If the comments are hard-coded in the markup above, that positioning is ignored.

Next steps

  1. Figure out what/why "so they won't inherit .text-content styles" means.
  2. Get a wrapper div around the comments in some other way.
CommentFileSizeAuthor
#20 After Patch 3165451.png266.64 KBchetanbharambe
#20 Before Patch 3165451 Two.png339.79 KBchetanbharambe
#20 Before Patch 3165451 one.png371.59 KBchetanbharambe
#18 3165451-olivero-node-template-comments.patch1.31 KBal0a
#14 3165451-olivero-node-template-comments-14.patch897 bytesaaron.ferris
#13 3165451-olivero-node-template-comments-13.patch918 bytesaaron.ferris
#5 After_patch.png350.74 KBvikashsoni
#5 Before_patch.png266.28 KBvikashsoni
#3 3165451-3.patch601 bytesgauravvvv

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Why are we disallowing location of comments within field ui » Olivero should not disallow the location of comments within field ui
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 9.1.x-dev
Component: Code » Olivero theme
Assigned: mherchel » Unassigned
gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3165451-3.patch601 bytes

I have attached a patch for the same. Please review.

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
vikashsoni’s picture

vikashsoni commented
StatusFileSize
new Before_patch.png266.28 KB
new After_patch.png350.74 KB

@Gauravmahlawat i think u have to check this patch because it's repeating comment form twice and this is very confuse able
sharing the screenshot..

gauravvvv’s picture

gauravvvv commented
dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Title: Olivero should not disallow the location of comments within field ui » Olivero should allow the placement of comments via field ui
Issue summary: View changes
dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Issue summary: View changes
dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Issue summary: View changes

Rewrote the ticket description and title to better illustrate

gauravvvv’s picture

gauravvvv commented
Status: Needs review » Needs work
tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: Unassigned » tushar_sachdeva
tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: tushar_sachdeva » Unassigned
aaron.ferris’s picture

aaron.ferris commented
StatusFileSize
new 3165451-olivero-node-template-comments-13.patch918 bytes

Would something like this work? Ill confess im not sure I understand the reasons for the {# Comments not part of content, so they won't inherit .text-content styles. #} either. Even with the template as it is, comments are inheriting .text-content styles so im slightly confused.

Im not confident the patch is adequate, but it'd resolve the problem of comments not being positioned from the field_ui and gives the comment element a specific targetabble ID.

Main question I have is, is {{layout}} needed on the comment wrapper class if the parent element has a {{layout}} class?

aaron.ferris’s picture

aaron.ferris commented
StatusFileSize
new 3165451-olivero-node-template-comments-14.patch897 bytes

Looking at the markup with the previous patch applied, I don't think we want the layout classes nested but im not 100%.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Version: 9.1.x-dev » 9.3.x-dev
Status: Needs review » Needs work

As noted in #3213214, we should also move the id="comments" to the comments field template so it's still present when comments are displayed in a block via layout builder.

xjm’s picture

xjm
she/her
Primary language English
commented
Category: Task » Bug report
Priority: Normal » Major

This definitely a bug. It might be major. Promoting for visibility until we make that decision.

What happens when the comments are placed in a block with Layout Builder?

al0a’s picture

al0a
he/him
Primary language Dutch
commented
StatusFileSize
new 3165451-olivero-node-template-comments.patch1.31 KB

Added a new patch where the id 'comments' is set in the field template.

al0a’s picture

al0a
he/him
Primary language Dutch
commented
Status: Needs work » Needs review
chetanbharambe’s picture

chetanbharambe commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before Patch 3165451 one.png371.59 KB
new Before Patch 3165451 Two.png339.79 KB
new After Patch 3165451.png266.64 KB

Verified and tested patch #18.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Olivero theme
# Goto: /admin/structure/types/manage/article/display
# Drag and drop the comment field at the top side or whatever you want.
# Create article content type
# Save it and check the results.

Expected Results:
# After applying the patch, the User is able to see placement of comments via field UI.

Actual Results:
# Currently, the Placement of comments via field UI is not working.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/olivero/templates/content/node.html.twig
@@ -105,12 +105,6 @@
-    <div id="comments" class="{{ layout }}">

+++ b/core/themes/olivero/templates/field/field--comment.html.twig
@@ -29,7 +29,7 @@
-<section{{ attributes.setAttribute('data-drupal-selector', 'comments').addClass('comments') }}>
+<section{{ attributes.setAttribute('id', 'comments').setAttribute('data-drupal-selector', 'comments').addClass('comments') }}>

Why do we have the ID attribute here anyway? I'm not sure it is correct in either case. If we display two nodes on a page both with comments then we're going to get duplicate IDs. As far as I can see this ID is not at all necessary. Bartik's print style sheet claims that that theme uses #comments but it's not true. If you have comments there is no div with an ID of comments. So why are we doing this in Olivero?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.