Closed Bug 956892 Opened 6 years ago Closed 6 years ago

collapsed comments should display tags to indicate the reason for auto-collapsing

Categories

(Bugzilla :: Creating/Changing Bugs, enhancement)

4.5.1
enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: asqueella, Assigned: glob)

References

Details

Attachments

(2 files, 2 obsolete files)

bug 793963 implemented the ability to tag comments. Per https://wiki.mozilla.org/BMO/comment_tagging#Implementation_Notes comments with certain tags (e.g. 'spam', 'obsolete') are automatically collapsed.

Collapsed comments look like this:

>> Nickname 2011-06-22 01:47:27 PDT  [tag] [reply] [+] Comment 99

...with no indication as to why they are collapsed.

I think it would be useful to show the comment's tags on collapsed comments:

>> Nickname 2011-06-22 01:47:27 PDT (spam) [tag] [reply] [+] Comment 99

to hint at the reason they are collapsed.
This is bmo only (see "BMO" in the URL) - moving.
Assignee: ui → nobody
Severity: normal → enhancement
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Version: unspecified → Production
This is an upstream feature, available in Bugzilla 4.5.2.
Assignee: nobody → create-and-change
Component: User Interface → Creating/Changing Bugs
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Version: Production → 4.5.1
Actually, I somewhat disagree with this, and would actually suggest that collapsed comments be made even more subtle. But we might end up with a similar end result.

It's good, effective moderation policy to minimize the aftermath of a problem, and avoid drawing attention to disturbances. Not to mention just keeping things clean and orderly.

A minimal change to get closer to what I'm thinking would be to change collapsed comments from:

>> Nickname 2011-06-22 01:47:27 PDT  [tag] [reply] [+] Comment 99

to:

>> Spam comment removed. [+]

or even just

>> Comment removed. [+]

Bonus points for some text style to easier to skim over (eg gray text, smaller font, etc tbd).

Double bonus points for collapsing multiple comments together ("5 comments removed"), but that's a different bug. :)
dolske: I agree completely. The reason I asked for tags to show up on collapsed comments was:
1) to make it clearer why it was collapsed (because it had "bad" tags) - your suggestions both make that clear.
2) to distinguish between useless comments (spam/advocacy) and obsolete comments that may have some value when reading through an old bug.
Assignee: create-and-change → glob
Attached patch 956892_1.patch (obsolete) — Splinter Review
- comments which are collapsed by default (ie tagged with any of the collapsed_comment_tags tags) now have a much simpler header
- the "comment hidden" text has a tooltip with the comment author and time
- expanding a comment will restore the standard header
Attachment #8418608 - Flags: review?(gerv)
Comment on attachment 8418608 [details] [diff] [review]
956892_1.patch

Review of attachment 8418608 [details] [diff] [review]:
-----------------------------------------------------------------

The graphic looks nice :-)

Gerv

::: template/en/default/bug/comments.html.tmpl
@@ +116,4 @@
>        <div class="[% class_name FILTER html %]">
>          [% IF mode == "edit" %]
>            <span class="bz_comment_actions">
> +            <span class="[% "collapsed" IF comment.collapsed %]">

This will produce a random empty <span> if the comment is not collapsed, i.e. in the common case. For page weight reasons (every little helps!) isn't it better to have the whole span conditional on comment.collapsed? Or were you trying to avoid two IF tests?

@@ +173,4 @@
>            [% END %]
>          </span>
>  
> +        <span class="bz_comment_time [% " collapsed" IF comment.collapsed %]">

If all these added "collapsed" classes are used to hide things, can't we put the class on a node higher up (probably the <div> that you are already adding bz_default_collapsed to) and then use descendant CSS rules to collapse the things we want?

div.collapsed > .bz_comment_time, ... { display: none; }

or whatever? (If the "collapsed" class is already used elsewhere for directly collapsed things, call it "collapsed_comment" instead.) This would also, AIUI, avoid the new loop in the expand_comment() function.

@@ +177,4 @@
>            [%+ comment.creation_ts FILTER time %]
>          </span>
> +
> +        [% IF comment.collapsed %]

So we are treating comments collapsed when the page is loaded differently to comments collapsed by the user?

Not objecting, just asking.

@@ +179,5 @@
> +
> +        [% IF comment.collapsed %]
> +          <span id="cr[% comment.count %]" class="bz_comment_collapse_reason"
> +             title="[% comment.author.name || comment.author.login FILTER html %]
> +                    [%~%] [[% comment.creation_ts FILTER time %]]">

Doesn't this "~" mean the time will follow the author name without even a space?
Attachment #8418608 - Flags: review?(gerv) → review-
(In reply to Gervase Markham [:gerv] from comment #7)

> div.collapsed > .bz_comment_time, ... { display: none; }

that sounds good, i'll experiment with that.

> So we are treating comments collapsed when the page is loaded differently to
> comments collapsed by the user?

yes.  the goal here is to de-focus comments which are collapsed by default (especially those which are spam/offtopic/etc).  comments collapsed by the user while viewing a bug don't need this as the user has already seen them.

> > +                    [%~%] [[% comment.creation_ts FILTER time %]]">
> 
> Doesn't this "~" mean the time will follow the author name without even a
> space?

no.
i'll replace it with [%~ %] for clarity in my next revision.
Attached patch 956892_2.patch (obsolete) — Splinter Review
- <span>s no longer created for non-collapsed comments
- using css inheritence to hide comment parts instead of explicit classes
Attachment #8418608 - Attachment is obsolete: true
Attachment #8423630 - Flags: review?(gerv)
Comment on attachment 8423630 [details] [diff] [review]
956892_2.patch

bug 1013986 pointed out that automatically collapsed comments are not expandable unless you're logged in.

i'll fix that here since it's the same area of code and somewhat related.
Attachment #8423630 - Attachment is obsolete: true
Attachment #8423630 - Flags: review?(gerv)
Blocks: 1013986
Attached patch 956892_3.patchSplinter Review
- shows the [+] expander to logged out users for collapsed-by-default comments
Attachment #8426852 - Flags: review?(gerv)
Comment on attachment 8426852 [details] [diff] [review]
956892_3.patch

r=gerv.

Gerv
Attachment #8426852 - Flags: review?(gerv) → review+
Flags: approval?
Flags: approval? → approval+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   80c434b..384a1c6  master -> master

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   9916f57..dd10df6  master -> master
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 5.0
You need to log in before you can comment on or make changes to this bug.