Brought up by @gábor-hojtsy in https://drupal.slack.com/archives/CJT807H7T/p1616581269020400

Olivero doesn't have any type of visual label indicating that the comment form is a comment form. Bartik does have this.

CommentFileSizeAuthor
#35 comment.png140.39 KBkiran.kadam911
#35 Screenshot 2021-08-17 at 8.31.17 PM.png381.95 KBkiran.kadam911
#34 3205597-34.patch1.57 KBmherchel
#31 3205597-31.patch718 bytesdjsagar
#27 After Screenshot.png148.85 KBRoshaniBhangale
#27 Before screenshot.png144.67 KBRoshaniBhangale
#24 Olivero_comment_form_should_have_title3205597-24.patch718 bytesmarcusvsouza
#24 reroll_diff_14-24.txt803 bytesmarcusvsouza
#23 After_Patch_3205597-14.png1.03 MBAgnesh Tank
#23 before_patch_3205597-14.png1.15 MBAgnesh Tank
#21 After Patch 3205597.png249.59 KBchetanbharambe
#21 Before Patch 3205597.png275.81 KBchetanbharambe
#20 commentafter.png128.3 KBRinku Jacob 13
#20 commentbefore.png115.14 KBRinku Jacob 13
#16 after_patch_#14.png359.32 KBkiran.kadam911
#16 before_patch_#14.png363.22 KBkiran.kadam911
#14 3205597-14.patch669 bytesPrabhat Burnwal
#14 interdiff-14.txt669 bytesPrabhat Burnwal
#14 3205597-14.patch669 bytesPrabhat Burnwal
#11 interdiff-11.txt650 bytesmonojithalder
#11 3205597-11.patch821 bytesmonojithalder
#9 After_appy_patch.png193.85 KBvikashsoni
#9 Before apply_patch.png204.38 KBvikashsoni
#7 3205597-after.png12.57 KBAbhijith S
#7 3205597-before.png17.76 KBAbhijith S
#5 Screenshot 2021-03-26 at 11.05.58.png45.29 KBGauravvvv
#5 3205597-5.patch669 bytesGauravvvv
#4 Screenshot 2021-03-26 at 11.02.41.png49.17 KBGauravvvv
bartik-comment-heading.png50.28 KBmherchel
olivero-comment-heading.png62.83 KBmherchel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

Gábor Hojtsy’s picture

Thanks! I thought Olivero may actually be doing something to actively undo this title? Or just does not contain it in the template?

mherchel’s picture

🤷‍♂️ No clue without investigating. Either way, we need to do what Bartik does.

Gauravvvv’s picture

Hi @mherchel, Now we're using the logic when we have comments then it will print the label, other wise it will not print it.

{% if comments and not label_hidden %}
    {{ title_prefix }}
    <h2{{ title_attributes.addClass('comments__title') }}>{{ label }} <span class="comments__count">{{ comment_count }}</span></h2>
    {{ title_suffix }}
  {% endif %}
Gauravvvv’s picture

I have removed the {{ comments }} the condition. Now comment label is visible without any comments. Added a patch and after patch screenshot for ref.

Gauravvvv’s picture

Status: Active » Needs review
Abhijith S’s picture

FileSize
17.76 KB
12.57 KB

Applied patch #5 and it works fine.

Before patch;
before

After patch:
after

RTBC +1

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Question is do we want the same title and the 0 counter or the different "Add new comment" title? How does the same form look like when there are comments already? (In Bartik and Olviero?)

vikashsoni’s picture

FileSize
204.38 KB
193.85 KB

Applied patch # 5 working but showing only comment label not Add new comment lable sharing screenshot....

mherchel’s picture

Pretty sure we can just copy over the comment field template from Classy. See https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/themes/barti...

monojithalder’s picture

New Patch updated

monojithalder’s picture

Status: Needs work » Needs review

New Patch updated

Status: Needs review » Needs work

The last submitted patch, 11: 3205597-11.patch, failed testing. View results

Prabhat Burnwal’s picture

Prabhat Burnwal’s picture

kiran.kadam911’s picture

Patch #14 applied successfuly and It's working fine.

Before patch SS:

After patch SS:

RTBC +1

Thanks!

lauriii’s picture

Issue tags: +Usability

Based on #8, we probably should review this in one of the UX meetings.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Moving to needs review based on #17.

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Rinku Jacob 13’s picture

FileSize
115.14 KB
128.3 KB

Patch #14 applied successfuly.

chetanbharambe’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
275.81 KB
249.59 KB

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

Testing Steps:
# Goto: admin/structure/types/manage/article/fields
# Click on Add field
# Select a Comment from "Add a new field"
# Mention the Label name
# Add into link field
# Save it.
# Create a Article content
# Save it
# User is not able to see "Comments" title on view page.

Expected Results:
# User should see "Comments" title on view page.

Actual Results:
# User is not able to see the "Comments" title on view page.

Looks good to me.
Can be a move to RTBC.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Based on #17, we still need to address #8.

Agnesh Tank’s picture

FileSize
1.15 MB
1.03 MB

Verified and tested patch #14.
Patch applied successfully and looks good

Testing Steps:


  1. Goto: admin/structure/types/manage/article/fields

  2. Click on Add field

  3. Select a Comment from "Add a new field"

  4. Mention the Label name

  5. Add into link field
 & Save it.

  6. Create a Article content
 & Save it

  7. verify the user is not able to see "Comments" title on view page.

Expected Results:
User should see "Comments" title on view page.
Actual Results:

User is not able to see the "Comments" title on view page.

This can be a move to RTBC.

marcusvsouza’s picture

I did a re-roll of the patch in comment #14, please revise!

andypost’s picture

The related issue could improve UX but it still needs usability review

RoshaniBhangale’s picture

Assigned: Unassigned » RoshaniBhangale
RoshaniBhangale’s picture

FileSize
144.67 KB
148.85 KB

Verified and tested patch #24 on the drupal 9.3.x-dev version. Patch applied successfully.

Testing Steps:

  • Goto admin/structure/types/manage/article/fields
  • Click on Add field
  • Select a Comment from "Add a new field" and Mention the Label name
  • Add into link field & Save it.
  • Create a Article content & Save it
  • Verify the user is able to see "Comments" title on page.

Expected Result:

  • User should see "Comments" title on page.

Actual Results:

  • User is able to see "Comments" title on page.

Please refer attached screenshot.
This is can be move to RTBC.

RoshaniBhangale’s picture

Status: Needs review » Reviewed & tested by the community
RoshaniBhangale’s picture

Assigned: RoshaniBhangale » Unassigned
mherchel’s picture

Status: Reviewed & tested by the community » Needs work

This is not passing tests, so should not be RTBC

djsagar’s picture

Status: Needs work » Needs review
FileSize
718 bytes

Rolling up patch.

mherchel’s picture

Priority: Normal » Minor
mherchel’s picture

It looks like we have a test that tests that the heading is not there!

[0;31m✖[0m Testing if element [0;33m<h2.comments__title>[0m is not present in 5000ms - expected [0;32m"is not present"[0m but got: [0;31m"present"[0m [0;90m(5127ms)[0m
[0;90m    at Object.Article without comments should not display count (/var/www/html/core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroCommentTest.js:31:19)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)[0m 
mherchel’s picture

kiran.kadam911’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
381.95 KB
140.39 KB

Patch applied successfully. Moving this to RTBC.

Before patch SS:

After patch SS:

Thanks!

  • catch committed 7b8ebde on 9.3.x
    Issue #3205597 by Prabhat Burnwal, mherchel, Gauravmahlawat,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7b8ebde and pushed to 9.3.x. Thanks!

Note I've removed some issue credit where manual testing was done on similar patches - once something has been manually tested with screenshots, it doesn't need doing so again on the same issue unless the approach radically changes.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.