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
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff.txt | 848 bytes | andypost |
#28 | commenting.png | 5.19 KB | andypost |
#24 | comments hidden from view.png | 87.38 KB | yoroy |
#24 | field type name.png | 47.33 KB | yoroy |
#24 | order of comment form settings.png | 79.4 KB | yoroy |
Comments
Comment #1
andypostCommit 2a6e875 fixes field settings are not saved but still require a test
Comment #2
kim.pepperSimple patch that renames the page title from "Comment bundle" to "Comment forms"
Comment #3
tsvenson CreditAttribution: tsvenson commentedThe wording changes in #2 works great for me and is RTBC.
Comment #4
andypostThanx commited
Comment #5
andypostI've started to write a formatters for field and here's a idea to move some settings to formatter
Comment #6
andypostPatch against current sandbox and new images
Field instance settings
Formatter settings (links)
Formatter settings (list)
Comment #7
andypostFull patch with formatters and widget
Comment #8
larowlanThis 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.
Comment #9
andypost@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.
Comment #10
andypost@larowlan Today I chat with @swentel he suggests to introduce hook_update_dependencies() to separate old and new field kinds
field_update_8002()
Comment #11
andypostCreated helper issue to test patches #1907960: Helper issue for "Comment field"
Comment #12
andypostAlso we can maintain
state()->set('comment.maintain_entity_statistics', TRUE);
on per instance basisComment #12.0
dixon_Updated issue summary.
Comment #13
dixon_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
Comment #14
dixon_Comment #15
andypost@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
Comment #16
andypost#1907960-176: Helper issue for "Comment field" shows that we need any router to attach fields, so per form settings seems useful
Comment #17
InternetDevels CreditAttribution: InternetDevels commentedAdded ability to change comments sorting order in the field settings.
Comment #18
andypostAdmin settings form screen
Comment #19
andypostUpdate field settings
Comment #20
andypost@tsvenson let's start to discus UX here, please
Comment #21
tsvenson CreditAttribution: tsvenson commented@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?
Comment #22
larowlan@tsvenson yep - I'm heading out now but will be back in a couple of hours if you want to discuss.
Comment #23
tsvenson CreditAttribution: tsvenson commented@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.
Comment #24
yoroy CreditAttribution: yoroy commentedA 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:
Comment #25
alexpottMarking critical now the patch is in
Comment #26
andypost@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
Comment #27
andypostManage fields #552604: Adding new fields leads to a confusing "Field settings" form
Comment #28
andypostWe 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'
Comment #29
tstoecklerMaybe we could make this two radios that show the two options?
Comment #30
andypostI'd prefer to have this checkbox in formatter settings
Also it's a bad idea to use radios for Boolean setting
Comment #31
jibranMoving it to core queue
Comment #33
klonosComment title should have three possible states: optional/required/disabled. This would get rid off the nasty "Allow" ;)
Comment #33.0
klonosClarified the summary a bit.
Comment #34
clemens.tolboomIn 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 :/
Comment #35
clemens.tolboomAdded XREF to Field Formatter
Comment #36
andypostbetter to fix this before beta
Comment #37
jhodgdonIs 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.
Comment #38
yched CreditAttribution: yched commentedre 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 ?
Comment #39
jhodgdonYeah, 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?
Comment #40
larowlan@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.
Comment #41
larowlanConsolidated into #1920044: Move comment field settings that relate to rendering to formatter options