Download & Extend
Issues

Remove collapsed comments and tidy up settings

Posted by catch on June 30, 2009 at 1:19pm
Project:Drupal core
Version:7.x-dev
Component:comment.module
Category:task
Priority:normal
Assigned:catch
Status:closed (fixed)
Issue tags:Quick fix, Usability

Issue Summary

This cleans up the comment settings form so it fits on one page.

Additionally collapsed comments are removed entirely.

collapsed comments are so 2001!

AttachmentSizeStatusTest resultOperations
comment_settings.patch13.56 KBIdleFailed: Failed to apply patch.View details
comment_settings_before.png487.01 KBIgnored: Check issue status.NoneNone
comment_settings_after.png448.36 KBIgnored: Check issue status.NoneNone
Login or register to post comments

Comments

#1

Posted by catch on June 30, 2009 at 1:24pm

Removed some descriptions.

AttachmentSizeStatusTest resultOperations
comment_settings.patch13.47 KBIdleFailed: Failed to apply patch.View details

#2

Posted by catch on June 30, 2009 at 1:40pm

Bit more form cleanup, new screenshot.

AttachmentSizeStatusTest resultOperations
comment_settings.png20.29 KBIgnored: Check issue status.NoneNone
comment_settings.patch13.57 KBIdleFailed: Failed to apply patch.View details

#3

Posted by Bojhan on June 30, 2009 at 2:42pm

Putting them inline, so I can comment.

#4

Posted by catch on June 30, 2009 at 2:45pm

When anonymous users have access to post comments:

Without the anonymous settings:

AttachmentSizeStatusTest resultOperations
settings_with_anon.png23.73 KBIgnored: Check issue status.NoneNone

#5

Posted by Bojhan on June 30, 2009 at 2:57pm

I think this is a really good cleanup, we could probally refine it a bit more. Almost looks like some functionality disappeared.

Do we need to mention that administer comments permission is able to override it? Not sure if Administrators will look here, to find that out - when it happens. (Not sure if it even causes confusion at their end)

Allow separate subjects when entering comments - not sure what is meant by separate here.

Quick reply form below comments - Can we reprhase this? Allow quick replying on comments? I like us to say we give the funcionality, without mentioning that an "form" is placed.

Also, I think Require preview should be the last option + unchecked by default.

#6

Posted by catch on June 30, 2009 at 3:20pm

Went through some of these with Bojhan, here's an updated patch.

AttachmentSizeStatusTest resultOperations
comment_settings.png21.59 KBIgnored: Check issue status.NoneNone
comment_settings.patch14.29 KBIdleFailed: 11813 passes, 13 fails, 0 exceptionsView details

#7

Posted by catch on June 30, 2009 at 3:37pm

Default the comment form to show below comments, I hate having to set this every time I install Drupal.

AttachmentSizeStatusTest resultOperations
comment_settings.patch16.17 KBIdleFailed: 11758 passes, 16 fails, 2 exceptionsView details

#8

Posted by kika on June 30, 2009 at 3:46pm

While we are at it:

t('Anonymous commenting'),

t('Anonymous posters may not enter their contact information'),
t('Anonymous posters may leave their contact information'),
t('Anonymous posters must leave their contact information')),

are too long and mentally hard to parse and distinguish. Any suggestions to simplify those?

#9

Posted by dmitrig01 on June 30, 2009 at 4:40pm

Quick Reply Form is somewhat vague. If i hadn't previoulsy read the issue i would have no idea what it meant.

#10

Posted by Bojhan on June 30, 2009 at 4:41pm

So, lets take a crack at those - lets step back and see the order.

Not allowed,
Allowed,
Required

So first of all it should be

Allowed,
Not allowed,
Required

Than what is confusing is they start with the same two words, but the distinction is only on the two following. Hmm, I dont have any suggestions to this yet.

#11

Posted by System Message on June 30, 2009 at 4:56pm
Status:needs review» needs work

The last submitted patch failed testing.

#12

Posted by catch on June 30, 2009 at 4:58pm
Status:needs work» needs review

I think quick reply is pretty standard now with gmail and others.

Had a crack at anonymous comments.

+        COMMENT_ANONYMOUS_MAY_CONTACT => t('Collect contact information'),
+        COMMENT_ANONYMOUS_MAYNOT_CONTACT => t('Do not collect contact information'),
+        COMMENT_ANONYMOUS_MUST_CONTACT => t('Require contact information')),
AttachmentSizeStatusTest resultOperations
comment_settings.patch18.65 KBIdleFailed: 11758 passes, 16 fails, 2 exceptionsView details
comment_contact.png19.93 KBIgnored: Check issue status.NoneNone

#13

Posted by System Message on June 30, 2009 at 5:06pm
Status:needs review» needs work

The last submitted patch failed testing.

#14

Posted by catch on June 30, 2009 at 5:14pm
Status:needs work» needs review

Fixed up the tests.

AttachmentSizeStatusTest resultOperations
comment_settings.patch19.39 KBIdleFailed: Failed to apply patch.View details

#15

Posted by catch on June 30, 2009 at 9:19pm

Put anonymous contact settings back how they were - let's do that in a followup patch - I don't have good ideas, and Bojhan reckoned the change was worse than the old text.

This also changes 'quick reply' to the less cryptic 'Show comment form on the same page as comments'.

AttachmentSizeStatusTest resultOperations
comment_settings.png21.12 KBIgnored: Check issue status.NoneNone
comment_settings.patch19.27 KBIdleFailed: 11665 passes, 57 fails, 29 exceptionsView details

#16

Posted by System Message on June 30, 2009 at 9:30pm
Status:needs review» needs work

The last submitted patch failed testing.

#17

Posted by yoroy on June 30, 2009 at 9:36pm
Status:needs work» needs review

Looking at the screenshot, all settings make sense and seem understandable to me (quick replies was the one I had difficulty understanding).
RTBC from a UI/UX perspective me thinks.

#18

Posted by Dries on June 30, 2009 at 9:57pm

Everything looks good, except that the test fails.

#19

Posted by catch on June 30, 2009 at 10:18pm

The new streamlined simpletest interface is great until you fix one test by breaking another. All comment tests should pass with this one.

AttachmentSizeStatusTest resultOperations
comment_settings.patch22.6 KBIdleFailed: 11734 passes, 4 fails, 2 exceptionsView details

#20

Posted by System Message on June 30, 2009 at 10:36pm
Status:needs review» needs work

The last submitted patch failed testing.

#21

Posted by catch on June 30, 2009 at 11:21pm
Status:needs work» needs review

grr.

AttachmentSizeStatusTest resultOperations
comment_settings.patch24.51 KBIdleFailed: Failed to apply patch.View details

#22

Posted by Bojhan on July 1, 2009 at 9:41am
Status:needs review» reviewed & tested by the community

#15 Looking at this, it looks RTBC to me. There are no improvements we can make from this point on, that should not live in its own followup.

#23

Posted by Dries on July 1, 2009 at 12:06pm
Status:reviewed & tested by the community» fixed

I'm cool with this. People that want collapsed comments, can use Javascript.

Committed to CVS HEAD. Thanks!

#24

Posted by System Message on July 15, 2009 at 12:10pm
Status:fixed» closed (fixed)

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

#25

Posted by kika on July 15, 2009 at 6:59pm
Status:closed (fixed)» active

There are still references to depreciated collapsed in modules/comment/comment-wrapper.tpl.php:

* - $display_mode
*   - COMMENT_MODE_FLAT_COLLAPSED
*   - COMMENT_MODE_FLAT_EXPANDED
*   - COMMENT_MODE_THREADED_COLLAPSED
*   - COMMENT_MODE_THREADED_EXPANDED

should become

* - $display_mode
*   - COMMENT_MODE_FLAT
*   - COMMENT_MODE_THREADED

#26

Posted by kika on July 15, 2009 at 7:18pm
Status:active» needs review

Here's the patch. I hope I got it in right format.

AttachmentSizeStatusTest resultOperations
comment-wrapper-cleanup.patch908 bytesIdlePassed: 11691 passes, 0 fails, 0 exceptionsView details

#27

Posted by catch on July 15, 2009 at 7:25pm
Status:needs review» reviewed & tested by the community
Issue tags:+Quick fix

I was sure I grepped everything, but apparently missed these.

#28

Posted by Dries on July 15, 2009 at 9:30pm
Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks.

#29

Posted by System Message on July 29, 2009 at 9:40pm
Status:fixed» closed (fixed)

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

nobody click here