Add setting to move comment form after comments

Will need some form of upgrade path for the new settings.

Also tagging for sub-maintainer for their thoughts.

Will need some form of upgrade path for the new settings.

Upgrade path is not needed in this case. The new setting is disabled by default and does not affect existing sites.

The upgrade hook should set the new config to false.

If I go to the olivero settings, change nothing, click save, and do a config export I'll get a new change now because the schema is different.

Upgrade hook added

Looks good for me!

Thanks folks we still need a subsystem maintainer opinion here - would suggest pinging one of the olivero maintainers in #frontend in slack

Left some comments on the MR - thanks folks

Not olivero maintainer, but it's ok with me.

nod_ → closed merge request !7409

Committed 26e593a and pushed to 11.x. Thanks!

nod_ → reopened merge request !7409

feedback incomming

Added feedback to the MR.

The config changes look v nice... a couple of other thoughts...

We can add an update test to \Drupal\Tests\olivero\Functional\Update\OliveroPostUpdateTest
I think we can test the functionality in core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroCommentTest.js

I bet it needs change record, moreover it reminds me that we have comment type entity to store this kind of data

Personally I find comment field formatter or comment type is a better place to store position of the form.

Also it needs to revisit D6/7 migrations as there was related setting

@andypost pointed out that the comment field already includes a setting for 'form location' but that's only if its on the same page or a separate reply page. I think its ok for a theme to make a setting for how to position the form when its on the same page.

    $element['form_location'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Show reply form on the same page as comments'),
      '#default_value' => $settings['form_location'],
    ];

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Added a "update test"

Also change record draft was created https://www.drupal.org/node/3467555 →

+1 to get in, just can't test 11.x with Bartok properly for BC but it's contributed theme now

Hi
I have reproduced the issue on Drupal 11 with the Olivero theme.
But the MR is falling for Drupal 11

Testing steps:
Just create a new node with comments

Attaching screenshot for reference

@Kanchan Bhogade MR successfully applied to 11.x branch. See https://git.drupalcode.org/issue/drupal-3439844/-/pipelines/251360

Hi
I have reproduced the issue on Drupal 11 with the Olivero theme.
The MR is applied cleanly...

Testing steps:
Just create a new node with comments

Test Result:
I have added 2 comments, but after applying the MR, the added comments are not visible only the count of comments is visible.

Attaching Screenshots

I cannot reproduce issue from #34

All works as expected

Attaching Screenshots too

Don't think this has been reviewed by an Olivero maintainer yet so tagging for that.

I think this is really something that should be configurable centrally in Drupal core, probably as a formatter setting. However we'd still need to remove the hard-coding of the form position in Olivero if we do that, but it would mean the position moves to the formatter settings instead of Olivero. Moving back to needs review for discussion.

should be configurable centrally in Drupal core, probably as a formatter setting

++ to get more opinions as there setting and default value when new theme enabled is tricky and same time option for comment form gonna be added in ✨ Allow a sitebuilder to change the Comment form heading Needs work

Re #27

theme to make a setting for how to position the form when its on the same page

consider a case when you have 2 comment fields for the entity and and wanna configure it at comment field/type level...

Looking back I find @catch choice is preferable as initial intent was #1920044: Move comment field settings that relate to rendering to formatter options →

Hey, folks! I don't agree about formatter. 99% themes render comment form after comments list, only olivero before. Formatter setting will not be used by most users. And not every olivero users too.

I use olivero in my blog and formatter setting is not relevant for me. If this issue will not be commited to olivero, I have to refuse this issue :(

99% themes render comment form after comments list, only olivero before.

Some sites will want to render the comment form before the comments and will be overriding themes, so the formatter would help in that direction too.

Yes, I understand, but I don’t have enough time for it

Hello
I have reproduced the issue for Olivero theme on Drupal 11.
The MR is applied successfully.

Testing steps-
Created new node and added comments.

Test Result:
Results are same before and after applied MR.
Check attachments.

I think it all means that Olivero doing too many things that is not ready in comment module, like hardcoding comment form placement and comment count ✨ Support per comment field comment count tokens Active via https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/oliver...

@sagarmohite0031 why did you remove all the tags?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed PHPStan issues reported by the bot in #51 but still some tests are failing that need to be fixed.

Thanks!

Rebased