Allow a sitebuilder to change the Comment form heading

Also we always get this link on node page

Agreed

@TODO: Translations

@GaborHojtsy - can you please explain `1) the list heading (defaults to "Comments")` little bit more, I think, this is field setting.

Fixed error from test.

Error was in missing form_heading in Comment test suite even when it was failing on Search module test.

Enabling translation for Form heading property.

Thanks for the patch @mirom. Just spotted a few nits. Could you fix them, please?

1. +++ b/core/modules/comment/src/CommentTypeInterface.php
   @@ -33,6 +33,24 @@ public function getDescription();
   +   * @return $this
   

   Missing return description?

Sorry, this nit was missing in previous comment.

+++ b/core/modules/comment/src/CommentTypeForm.php
@@ -101,6 +101,13 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#default_value' => empty($comment_type->getFormHeading())?'Add new comment':$comment_type->getFormHeading(),

Missing spaces.

Fixing whitespace errors.

I think testbot will like the patch so it's RTBC for me. Checked with @mirom in Drupalaton :)

Great job!

@xano and @dawehner have been wondering whether instead of using a configuration we could pass along the context to the t() call depending on the comment type.
This then would allow you to translate it per context, or?

What should be context in this case? I see configuration more clear then context translation.

Gabor says, the proposal in #21 is hack, so back to RTBC.

Almost there, good job @mirom!
I found one small thing:

+++ b/core/modules/comment/src/CommentTypeForm.php
@@ -101,6 +101,13 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#default_value' => empty($comment_type->getFormHeading()) ? 'Add new comment' : $comment_type->getFormHeading(),

The static text 'Add new comment' should be wrapped in t().

Thanks @zserno, fixed.

Works for me with the latest patch. RTBC+1
D

+++ b/core/modules/comment/src/CommentTypeForm.php
@@ -101,6 +101,13 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#title' => t('Form heading'),
...
+      '#default_value' => empty($comment_type->getFormHeading()) ? t('Add new comment') : $comment_type->getFormHeading(),

These can use the injected t(). aka: $this->t()

This likely needs to be pushed to 8.1.x since we've past RC. Assigning to webchick for product feedback.

Also this is not a bug, this a feature request. You can change that text through Translation in settings.php and by extending the template. This is a site builder feature request

We shouldn't do these type of settings through the UI. Submissions is a completely different type of setting, as thats about supplying guidelines.

I am going to won't fix this, we don't do this through the UI no point in starting to do that now (especially not as a one-off).

@Gábor How do we deal with this in other places? We don't offer this type of configurability for other types of UI facing elements that are module provided right? Only for fields. And this is not like a "field group" its a special legacy thing, from the old days.

I would argue that first setting is even worrisome. I saw your twitter message, and I think the whole point of our theme system is to go to the template files when you have to change things like that. We shouldn't have a UI for every user facing string (unless its translation), only the "flexible" parts.

At the very least we should have a standard, not one-offs.

What is broken here? Nothing. The fact that we have a one-off for one user interface element, does not necessarily constitute it also being there for the 2nd one. I feel like we discussed this particular feature before, and decided that the usecase for the 2nd one was ever so small.

This is adding a "thing" not about making it more consistent, because we don't have a pattern for this in core.

To me, the screenshot / description in #33 is definitely describing a bug. However, I agree with Bojhan that adding a UI setting for that particular piece of text feels very odd. As opposed to simply putting in the twig file:

<h2>Add new {{entity.label}}</h2>

...or whatever.

That would solve the crux of this issue, without presenting new and confusing, one-off UI options.

Ok ok :) Sorry for moving this about so much.

@Gabor - what do we still need to fix in this issue? Should it go back to RTBC and be commited into 8.1.0?

As I said in #1 we also need to take care about node links (maybe separate issue) because now there's a bug - "Add new comment" link is always displayed (in teaser and full node view).
We should be able to configure node link too!

The only thing I'm not sure here is a usage of comment type to store this data.
Comment types are not about fields, they are about comment relation to commented entity!
So I think that field (or field storage) setting is most suitable for that because formatter and comment form are bound to the field not the comment type.

@mirom This could be accepted only as a part of 8.1 and later, and this needs hook_update_n() implementation to update all existing comment types.

1. +++ b/core/modules/comment/comment.module
   @@ -763,6 +763,10 @@ function comment_preprocess_field(&$variables) {
   +    $comment_type = CommentType::load($variables['comment_type']);
   +    $variables['form_heading'] = $comment_type->getFormHeading();
   

   I'd like to get rid of entity loading in preprocess

2. +++ b/core/modules/comment/src/CommentTypeForm.php
   @@ -101,6 +101,13 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#title' => t('Form heading'),
   ...
   +      '#default_value' => empty($comment_type->getFormHeading()) ? t('Add new comment') : $comment_type->getFormHeading(),
   
   +++ b/core/profiles/standard/config/install/comment.type.comment.yml
   @@ -2,5 +2,6 @@ id: comment
   +form_heading: 'Add new comment'
   

   $this-t() should be used in forms

   and there's no reason for empty() check - default value should be provided by comment type entity (as standard profile does)

Agree with @andypost on this - we want to make these (plugin based) formatter settings in #2377849: Simplify what CommentLinkBuilder is doing - consider removing a lot of the functionality so it would align with that.

Do we even need the Add new comment? Can't we simply remove it?

@wim leers

Do we even need the Add new comment? Can't we simply remove it?

IMO we do not need it. The "comment" label over the text area is sufficient.

While we're at it we really don't need "subject" either. :)

If you have multiple comment types on the page, which is a total usecase, dropping those labels makes the situation just worse.

Note, this small module was needed to make a page with two comment types usable. There are other places like the entity edit form, which has similar problems.

While we're at it we really don't need "subject" either. :)

Indeed. But there's a separate issue for that already: #2312329: Hide the comment "subject" field by default.

If you have multiple comment types on the page, which is a total usecase, dropping those labels makes the situation just worse.

But then you still have the "comment type" label?

We'd just change from this:

Comments
…
…
Add new comment
[FORM]

Product Feedback
…
…
Add new comment
[FORM]

to this:

Comments
…
…
[FORM]

Product Feedback
…
…
[FORM]

Is this bug going to be fixed?

As it stands if you have 2+ comment forms on a page you cant label them? Such a simple thing is expected from a UI/UX perpective?

:)

updating issue summary based on review with YesCT @Drupalaton 2016
will try to do the remaining tasks tomorrow

Here's some templates cleanup and

+++ b/core/modules/comment/src/CommentTypeForm.php
@@ -95,6 +95,13 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#default_value' => $this->t($comment_type->getFormHeading()),

Fixed translatable config usage

This needs upgrade path

I rerolled the patch, but I don't see the form header appearing.

Patch in #63 worked for me

reroll + fix for tests

Cleaner default value

Add update hook & fix forum default config

Still needs work to extend tests & upgrade path tests

I installed the patch in #70 over Drupal 8.7.3 using Composer. When I then ran update.php I got a white screen with this error:

Fatal error: Cannot redeclare comment_update_8701() (previously declared in C:\Users\Nick\Sites\seelife\web\core\modules\comment\comment.install:255) in C:\Users\Nick\Sites\seelife\web\core\modules\comment\comment.install on line 274

But the site still functioned and I was able to remove the patch and successfully run update.php.

Perhaps I need to reverse some of my site's comments configuration before applying the patch and running update.php, but I'm not sure what.

In the meantime I achieved it by commenting out the "Add new comment" line in a custom template, per Shawn Conn's answer here.

Yep, it should be post update hook instead of hook update

Hi @andypost, rerolled the patch in #70. The changes made in core/modules/comment/comment.install is not included in the rerolled patch. After the changes made in #3087644: Remove Drupal 8 updates up to and including 88**, I am not sure how to go about it...

@ankithashetty Thank you! the update hook needs re-work using hook_post_update_NAME() ref https://www.drupal.org/node/2563673

+++ b/core/modules/comment/comment.install
@@ -220,3 +220,14 @@ function comment_update_8700() {
+function comment_update_8701() {
+  $config_factory = \Drupal::configFactory();
+  foreach ($config_factory->listAll('comment.type.') as $type_name) {
+    $type = $config_factory->getEditable($type_name);
+    $type->set('form_heading', 'Add new comment')->save(TRUE);
+  }
+}

this hunk should go to comment.post_update.php as it changing config (doing upgrade)

As example you could use views_post_update_field_names_for_multivalue_fields()

Hey @andypost, thank you for all the references... I updated the patch as per your suggestions and fixed the custom failure errors in #79, kindly review.

Looks great! It still needs additional tests for new functionality and test for upgrade hook

Adding a patch with test that will verify the functionality of new configurations and also made some changes in code/themes/bartik and core/theme/seven for comment field which were missed.

Changes invalid username issue in the previous patch and corrected unequal array in test.

Updated coding standards.

The issue is a feature, but I see more like UX now

Added reroll of patch #86 on Drupal 9.4.x.

Updated patch will fix test failures & added an interdiff as well.

Hope this will fix test failure.

Marked #2978209: Replace hard coded Add new comment in field--comment.html.twig as duplicate and moving credit over here.

I became aware of the issue when @andypost mentioned on the #bugsmash channel that there is the need of an ux review for a while. I've applied that patch to a Drupal 9.4.x install yesterday night and ran into a possible issue i am not quite sure if it is rooted in this patch, if it is a separate Olivero issue or if it is simply based on a configuration mistake of mine. I've agreed with @andypost to comment with the infos i've previously posted in the Slack chat. and when the point about Olivero (1.) is figured out i will propose the issue for the weekly UX meeting that more pair of eyes and perspectives take a look at the issue. I am not comfortable to remove a needs usability review tag on my own and anyway it is always better to have more people with different perspectives and background discuss the issue. about the points i've noticed.

1. I've used Claro as the admin theme and Olivero as the front end theme when setting up a comment type. For the Form heading i've used Add a new thought while i've used Your thoughts as the label for the comment field in the article content type and You add your thought in the comment field as the help text. I've then created an article and added a new comment called my very first comment with test in the body. but the visual representation is confusing:

First comes the title Your thoughts , then the new comment entry form missing the Add a new thought form heading and then the list of comments. Also the help text isn't shown anywhere? ( I am not sure if that is the expected behaviour in regards of the help text).
I've then switched with the same configuration to Bartik. The output looks like that now:

the comment entry form is correctly showing the set title and the order is different (in a way i'Ve expected). but the help text is missing there as well.

so is it possible that for Olivero the comment entry form title has to be added to the twig file as well? and is the positioning of the comment entry form differing between Olivero and Bartik? Meaning that in Bartik it is placed at the end while for olivero at the beginning? and as i said the help text is missing for both. But the description says Instruction to present to the user below this field on the editing form, so i would expected it to be shown in the front end?

2. The label for the introduced field Form heading is a bit unspecific and general imho. If I wouldn't have had the context from the IS i would have had to make assumptions based on the default text Add new comment. But it would be probably helpful to have a clear title and perhaps explanatory description as well?
I am not a native speaker therefor hesitant to make any definite word smithy suggestions. But something in the direction of New comment entry form heading. That would be clearer but also too lengthy unfortunately.

3. And the last point is about the placement of the field. When i've applied the patch i've searched in the comment field settings (/admin/structure/types/manage/article/fields/node.article.comment) the screenshots in the IS were about. also it is the place where the title of the comment section is set . therefor i thought i would expect the field to be found there. that the user is able to set the title for the add new comment form in the same context on the same page.
took me quite a while, and i already thought even though the patch successfully applied that something is wrong with it and it failed to provide the functionality it wants to provide, until i've realised the field is located on the edit page of a comment type /admin/structure/comment/manage/comment?destination=/admin/structure/comment. I am not sure if it would be possible and also make sense from a developer point of view to move the field to the comment field page? or what others think about that? at least for me personally it was quite confusing and an unexpected location and placement of the field.

so is it possible that for Olivero the comment entry form title has to be added to the twig file as well? and is the positioning of the comment entry form differing between Olivero and Bartik?

Yeah, it looks like we change that around in our field--comment.html.twig. I'd be fine going back to the default, but we should talk to the UX group about it and then maybe put together a design for the small "Add new comment" jump link that appears by default.

Hopefully this answers all the Olivero related questions here. Let me know if it doesn't.

Thanks for working on this!

I've presented the issue on yesterdays usability meeting (2022-05-06). The list of attendees was @AaronMcHale, @benjifisher, @bnjmnm, @ckrina, @shaal, @worldlinemine and me. The results are as follows:

1. The label of the form field alone is a bit vague. The group agreed to add the following description to the field to provide more clarity (the wording is based on the style of the other descriptions) : This text will be displayed as the heading of the comment form, if the theme supports it.. The second part of the sentence (after the comma) was chosen and added because it isn't sure if a frontend theme is supporting the display of the form heading field at all. And it might be confusing for the users if they add content to the form heading field and the entered text couldn't be found in the front end theme afterwards (like it is currently the case with Olivero).

2. The point I've made in #102.2 that the label form heading might be adjusted was discussed as well but it was decided against it. The combination of the label, the placeholder text Add new comment and the newly introduced description This text will be displayed as the heading of the comment form, if the theme supports it. provides enough context and clarity. So the label could remain as is.

3. The group then agreed if it would be technically possible to move the form heading field from the comment types page over to the comment field page for a content type. So the form heading is configurable along with the display label for the comment field. We haven't talked or agreed on the exact position on the comment field page. I just did a quick mockup by adding the form heading component in devtools to the page:

Addendum: While writing the comment @benjifisher commented the screenshot i've posted alongside in the #ux channel and agreed that the position of the form heading field is good: "near the other text fields and above the other form elements"

4. When checking the latest patch in #96 the variable form_heading is added to the field--comment.html.twig in Bartik and wrapped in a h2. At the moment field--comment.html.twig in Olivero doesn't contain the same addition of the form_heading variable wrapped in a h2. That way the effect of the patch isn't visible to users who have selected Olivero as their default theme, which could create or lead into confusion. So it would be good to add it for Olivero as well.

@mherchel i've noticed that i have forgot to bring up the order of the comment components in yesterdays call. sorry about that. :( but i think that detail isn't directly related to this issue. therefor we could revisit in one of the next ux meeting calls and create a follow up issue in case the rest of the group considers it an issue as well? You think that would be ok?

Thank you for all the work on the issue, and definitely a good and useful addition to Core. :)

Usability review was provided in comment #104, removing tag.

I've updated the task description based on the UX committee's recommendations.

As the Comment Module's maintainers, @larowlan or @andypost can you review the proposed resolution and state whether you approve of the proposed resolution? And if not, list out your concerns and ideally an alternative proposal?

Updated the issue branch to make it merge-able again since conflicts arising from https://www.drupal.org/project/drupal/issues/3396165