Problem/Motivation

Over at #731724-284: Convert comment settings into a field to make them work with CMI and non-node entities the comment entity is getting a dedicated page for all it's different bundle (based on the different comment fields attached to entities). For now that page is called "Comment forms".

This UI needs improvements in terms of UX to make it clear for the user what these bundles are being used for and to make sure it's easy to find and update comment settings.

Proposed resolution

The bundle overview pages as well as field instance and formatter settings needs reviews in terms of layout and terminology.

Some code needs to be moved over to formatter settings, that is happening here: #1920044: Move comment field settings that relate to rendering to formatter options

Original report by andypost

There's a some cases to take into account:
1) Most of sites have global and separate settings for commenting form!
1.a) this setting could live on per field level - moving to field annotation?
1.b) introduce config entity to provide defaults a-la image-style?
2) threading and per page goes to CommentList formatter settings - there's some issue to fix tests. I'll attach patch latter in #1901110: Improve the UX for comment bundle pages and comment field settings
3) Place to display the Comment Form - suppose this move to CommentList formatter settings too
4) comment_entity_view() - introduce second CommentLinks formatter with some hack to move comment links to node links

We should address this changes in patch

1) Administrative menu name
2) Overview screen columns
3) Field settings #284 - require fix
#285 - require fix and tests
#286 - require fix and tests

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Commit 2a6e875 fixes field settings are not saved but still require a test

kim.pepper’s picture

Status: Active » Needs review
FileSize
702 bytes

Simple patch that renames the page title from "Comment bundle" to "Comment forms"

tsvenson’s picture

The wording changes in #2 works great for me and is RTBC.

andypost’s picture

Thanx commited

andypost’s picture

I've started to write a formatters for field and here's a idea to move some settings to formatter

andypost’s picture

Title: Improve the UX for comment bundle pages and comment field settings » Comment UI fixes
FileSize
24.15 KB
24.36 KB
48.16 KB
29.99 KB

Patch against current sandbox and new images

Field instance settings
instance-settings.png

Formatter settings (links)
links-formatter.png

Formatter settings (list)
list-formatter.png

andypost’s picture

Full patch with formatters and widget

larowlan’s picture

This looks great on first inspection @andypost.
I'd hoped to punt this stuff to follow ups but having it done beforehand strengthens the original patch.
Are we leaving this out of 731724 until it is in (ie as a follow up?) or attempting to add it now?
Do you know if the tests still pass? I recall the comment links tests being very fragile.

andypost’s picture

@larowlan This changes could be done as follow-up, probably...
This highly depends on ability properly map node-teaser and other views modes to formatter settings. So maybe we need to fix it right in 731724

It needs more work to properly decouple field-instance-settings into views-modes and their formatter settings.

andypost’s picture

@larowlan Today I chat with @swentel he suggests to introduce hook_update_dependencies() to separate old and new field kinds
field_update_8002()

swentel: I'm already mixing up with a current patch which moves this to 8001 ;)
swentel is converting it to full cmi, patch is already 400kb, big fun :p
swentel: the conversion needs to happen at the very end, and also after field_sql_storage, at least, at this moment

andypost’s picture

Created helper issue to test patches #1907960: Helper issue for "Comment field"

andypost’s picture

Also we can maintain state()->set('comment.maintain_entity_statistics', TRUE); on per instance basis

dixon_’s picture

Project: Comment as field » Drupal core
Issue summary: View changes

Updated issue summary.

dixon_’s picture

Title: Improve the UX for comment bundle pages and comment field settings » Improve the UX for comment bundle pages

Let's narrow down the scope of this issue to just focusing on improving the UX workflow of the comment bundle pages and the field settings forms.

Some code needs to be moved, that's happening here: #1920044: Move comment field settings that relate to rendering to formatter options

dixon_’s picture

Title: Comment UI fixes » Improve the UX for comment bundle pages and comment field settings
Project: Drupal core » Comment as field
andypost’s picture

Title: Improve the UX for comment bundle pages » Improve the UX for comment bundle pages and comment field settings

@dixon_ I still sure that we need to move Form settings away from bundle so the only Default value will stay here.
OTOH the default value seems should be stored in our own key while #1919834: Field instance got no default value when created in field UI is broken
Another way is store form settings in comment bundles a-ka a kind of configurables

andypost’s picture

#1907960-176: Helper issue for "Comment field" shows that we need any router to attach fields, so per form settings seems useful

InternetDevels’s picture

Added ability to change comments sorting order in the field settings.

andypost’s picture

FileSize
9.78 KB

Admin settings form screen

field settings admin settings
field-settings.png admin-settings.png
andypost’s picture

Title: Comment UI fixes » Improve the UX for comment bundle pages and comment field settings
FileSize
43.42 KB

Update field settings

andypost’s picture

@tsvenson let's start to discus UX here, please

tsvenson’s picture

@andypost. Sure thing. Got stuck with other stuff last night, but will start testing in about 30-60 minutes.

I assume it is still the patch in #2079093: Patch testing issue for Comment-Field that is the relevant one?

larowlan’s picture

@tsvenson yep - I'm heading out now but will be back in a couple of hours if you want to discuss.

tsvenson’s picture

@larowlan. Brilliant. I think the best, and most productive to take full advantage of DrupalCon sprint, is that I take notes when going over it and then we have a screnshare hangout where we can discuss.

yoroy’s picture

A first review:

Minor: I noticed all field types use the singular for their name. For consistency, the field type should be 'Comment', not 'Comments'.

When adding a Comments field:

Possibly out of scope but the 'Hidden from view' option could be expressed a bit more clearly.

- Threading should be off by default because the 80% use case is just straight up comments without threading. - This option should come after the number of I think.
- 'Allow comment title' sounds sort of arrogant to me. We don't allow people to do things, we help, provide, support them to do things
- I think this title option should be off by default as well. I see more comment fields without titles than with.
- 'Show reply form on the same page as comments'. It's not entirely clear what the other option is here. On it's own page presumably. Again, isn't 'on the same page' the 80% use case? Then this checkbox should say 'Show the comment form on its own page' and by default be unchecked. (Checkboxes should as much as possibly be opt-in)

Manage fields

These links lead to that currently useless, non-actionable page about 'These settings apply everywhere…' There are no settings to change there. Are there cases where there will be? Because otherwise we shouldn't be linking to it in the first place.

Comment display

I ordered the comment field to be above the body field in my new content type but that's not how things got rendered in nodes of that content type:

alexpott’s picture

Priority: Normal » Critical

Marking critical now the patch is in

andypost’s picture

@yoroy
1. actually this field is Comments because it is a comment thread
2. Hidden was reverted to be shown because currently core does this (we are going to chenage this un follow-up to proper)
3. Suggestions welcome!
4. Yes, currently field ui does not allows to hide this, we have no field level settings now
5. Order is a core bug and there's a issue in queue that I can't find now ;(
6. Templates should use field label #1962846: Use field instance name for header of comment list, drop comment-wrapper template

andypost’s picture

andypost’s picture

FileSize
5.19 KB
848 bytes

We can get rid of 'Hidden' in field settings totally because it duplicates functionality for display and form settings.
But I think we should hold this settings on per-entity basis to allow hide comments for particular entity.

Same time we need a additional setting for singular to use for 'Add new {comment}'

'Comments' as field name probably better to rename to 'Commenting'
commenting.png

tstoeckler’s picture

'Show reply form on the same page as comments'. It's not entirely clear what the other option is here. On it's own page presumably. Again, isn't 'on the same page' the 80% use case? Then this checkbox should say 'Show the comment form on its own page' and by default be unchecked. (Checkboxes should as much as possibly be opt-in)

Maybe we could make this two radios that show the two options?

andypost’s picture

I'd prefer to have this checkbox in formatter settings
Also it's a bad idea to use radios for Boolean setting

jibran’s picture

Project: Comment as field » Drupal core
Version: » 8.x-dev
Component: Code » comment.module

Moving it to core queue

Status: Needs review » Needs work

The last submitted patch, comment-field-settings-1901110-17.patch, failed testing.

klonos’s picture

Comment title should have three possible states: optional/required/disabled. This would get rid off the nasty "Allow" ;)

klonos’s picture

Issue summary: View changes

Clarified the summary a bit.

clemens.tolboom’s picture

In both #2141929: Comment link or form is added to print view mode. and #1166114: Manage Displays Search Results doesn't manage the display of the search results we add code to hide the 'Add new comment' link depending on 'Display mode'.

It seems this is solved in #6

I'm puzzled with the issue summary having cryptic lists (three types of numbering) and no images which seems important enough.

OTOH I guess these are moved as mentioned in #13 to #1920044: Move comment field settings that relate to rendering to formatter options :/

clemens.tolboom’s picture

Added XREF to Field Formatter

andypost’s picture

Issue tags: +beta target

better to fix this before beta

jhodgdon’s picture

Is there any kind of a patch for this issue?

The reason I am asking is basically comment #34. We have this issue #1113832: [PP-1] Comment module renders "reply" and other links in search index/results where the Comment module is rendering links into the search index and search results, and we can either fix it via a kludgy mess of code (like what is there now to hide the comment form in these cases), or through having reasonable settings on the Comment module that can hide links and the comment form for the search results/indexing view modes.

I'm very confused by the issue summary and I'm not sure whether #34 applies to this issue or the referenced issue #1920044: Move comment field settings that relate to rendering to formatter options. Can we get an updated summary that explains what the difference is between the two issues, and whether one depends on the other please?

Also, if this isn't going anywhere anytime soon, we can procede with kludges in the other issue, as was done on #2141929: Comment link or form is added to print view mode.. I just need to know what to do, because we do need to resolve the search module issue one way or another.

yched’s picture

re search index :
node.module uses hook_entity_view_display_alter() to make sure field labels are not displayed when rendering content in the 'search_index' view mode (e.g. avoid indexing "Body:" for all nodes)
Maybe something similar could be done about comment fields ?

jhodgdon’s picture

Yeah, there are already several kludges in the node and comment modules that special case the search_index and search_results view modes. We can certainly add another one to deal with the links, as was done in #2141929: Comment link or form is added to print view mode. for print mode.

But if it could just be a display setting on the Comment field, it would seem that this would be a better solution? Maybe we should do that for the field labels as well for searching?

larowlan’s picture

@jhodgon yes this is still on our radar, but without it, we have to go kludgy and clean up later.
no sense in blocking your work.

larowlan’s picture

Status: Needs work » Closed (duplicate)