Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#35 | comment.png | 140.39 KB | kiran.kadam911 |
#35 | Screenshot 2021-08-17 at 8.31.17 PM.png | 381.95 KB | kiran.kadam911 |
#34 | 3205597-34.patch | 1.57 KB | mherchel |
#31 | 3205597-31.patch | 718 bytes | djsagar |
#27 | After Screenshot.png | 148.85 KB | RoshaniBhangale |
Comments
Comment #2
Gábor HojtsyThanks! I thought Olivero may actually be doing something to actively undo this title? Or just does not contain it in the template?
Comment #3
mherchel🤷♂️ No clue without investigating. Either way, we need to do what Bartik does.
Comment #4
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedHi @mherchel, Now we're using the logic when we have comments then it will print the label, other wise it will not print it.
Comment #5
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedI have removed the {{ comments }} the condition. Now comment label is visible without any comments. Added a patch and after patch screenshot for ref.
Comment #6
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #7
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #5 and it works fine.
Before patch;
After patch:
RTBC +1
Comment #8
Gábor HojtsyQuestion 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?)
Comment #9
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedApplied patch # 5 working but showing only comment label not Add new comment lable sharing screenshot....
Comment #10
mherchelPretty 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...
Comment #11
monojithalder CreditAttribution: monojithalder at Innoraft commentedNew Patch updated
Comment #12
monojithalder CreditAttribution: monojithalder at Innoraft commentedNew Patch updated
Comment #14
Prabhat Burnwal CreditAttribution: Prabhat Burnwal commentedComment #15
Prabhat Burnwal CreditAttribution: Prabhat Burnwal commentedComment #16
kiran.kadam911Patch #14 applied successfuly and It's working fine.
Before patch SS:
After patch SS:
RTBC +1
Thanks!
Comment #17
lauriiiBased on #8, we probably should review this in one of the UX meetings.
Comment #18
lauriiiMoving to needs review based on #17.
Comment #20
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedPatch #14 applied successfuly.
Comment #21
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedVerified 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.
Comment #22
lauriiiBased on #17, we still need to address #8.
Comment #23
Agnesh Tank CreditAttribution: Agnesh Tank at Acquia for Acquia commentedVerified and tested patch #14. Patch applied successfully and looks good
Testing Steps:
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.
Comment #24
marcusvsouza CreditAttribution: marcusvsouza at CI&T commentedI did a re-roll of the patch in comment #14, please revise!
Comment #25
andypostThe related issue could improve UX but it still needs usability review
Comment #26
RoshaniBhangale CreditAttribution: RoshaniBhangale at QED42 for Drupal India Association commentedComment #27
RoshaniBhangale CreditAttribution: RoshaniBhangale at QED42 for Drupal India Association commentedVerified and tested patch #24 on the drupal 9.3.x-dev version. Patch applied successfully.
Testing Steps:
Expected Result:
Actual Results:
Please refer attached screenshot.
This is can be move to RTBC.
Comment #28
RoshaniBhangale CreditAttribution: RoshaniBhangale at QED42 for Drupal India Association commentedComment #29
RoshaniBhangale CreditAttribution: RoshaniBhangale at QED42 for Drupal India Association commentedComment #30
mherchelThis is not passing tests, so should not be RTBC
Comment #31
djsagar CreditAttribution: djsagar at OpenSense Labs commentedRolling up patch.
Comment #32
mherchelComment #33
mherchelIt looks like we have a test that tests that the heading is not there!
Comment #34
mherchelComment #35
kiran.kadam911Patch applied successfully. Moving this to RTBC.
Before patch SS:
After patch SS:
Thanks!
Comment #37
catchCommitted 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.