Problem/Motivation

The page title for comment type edit form is just "Edit". This is pretty confusing because it doesn't contextualize the local tasks, but also leads into incorrect breadcrumbs on the subpages as reported in #2587415: Comment types UI confusing and inconsistent.

Proposed resolution

Change the page title to the current comment type.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3374626.png21.97 KBpenyaskito
#13 interdiff.txt1.35 KBlauriii
#13 3374626-13.patch2.66 KBlauriii
#9 interdiff.txt1.58 KBlauriii
#9 3374626-9.patch3.65 KBlauriii
#7 interdiff.txt2.2 KBlauriii
#7 3374626-7.patch3.51 KBlauriii
#2 3374626-2.patch2.1 KBlauriii
Screenshot 2023-07-14 at 2.01.51 PM.png74.83 KBlauriii

Issue fork drupal-3374626

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Active » Needs review
StatusFileSize
new 3374626-2.patch2.1 KB
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Agree the title could be updated.

I checked content types and media types
Content types = Edit Article content type
Media type = Edit Audio

So can we use Edit [comment-type-name] or Edit [comment-type-name] comment type

dhirendra.mishra made their first commit to this issue’s fork.

dhirendra.mishra opened merge request !4388

dhirendra.mishra closed merge request !4388

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
Issue tags: -undefined +Field UX
StatusFileSize
new 3374626-7.patch3.51 KB
new interdiff.txt2.2 KB

We might be adjusting or getting rid of some of these title overrides in #3370946: Page title should contextualize the local navigation but agree that it makes sense to be consistent here. 👍

Status: Needs review » Needs work

The last submitted patch, 7: 3374626-7.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
StatusFileSize
new 3374626-9.patch3.65 KB
new interdiff.txt1.58 KB

Couple small improvements after a self-review. 😇

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Title change looks good.

Edit Default comments comment type

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/comment/comment.routing.yml
@@ -135,7 +135,7 @@ entity.comment_type.edit_form:
+    _title_callback: '\Drupal\comment\Controller\CommentTypeController::commentTypeTitle'

I think this can use \Drupal\Core\Entity\Controller\EntityController::title then we don't need a custom controller or method.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue tags: +ddd2023
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
StatusFileSize
new 3374626-13.patch2.66 KB
new interdiff.txt1.35 KB

Makes sense 👍

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito as a volunteer commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3374626.png21.97 KB

LGTM. Surprised this wasn't reported until 2023.

Static review looks good. Actually tested the patch, attached screenshot.

dhirendra.mishra’s picture

dhirendra.mishra CreditAttribution: dhirendra.mishra at Axelerant for Axelerant commented

dhirendra.mishra opened merge request !4404

dhirendra.mishra’s picture

dhirendra.mishra CreditAttribution: dhirendra.mishra at Axelerant for Axelerant commented

Please ignore #16 , Created by mistake

  • longwave committed 95dcdb3f on 11.x 
    Issue #3374626 by lauriii, penyaskito, smustgrave, longwave: Comment...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 95dcdb3f58 to 11.x. Thanks!

Discussed with @lauriii at Drupal Dev Days and decided not to commit to 10.1.x as there is a new translatable string, it's a UI bug that has existed for a long time and it can wait for 10.2.x.

Status: Fixed » Closed (fixed)

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