Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

#4844 closed defect (bug) (fixed)

Show all X comments should update when comments are deleted

Reported by: modemlooper Owned by: boonebgorges
Milestone: 1.8 Priority: low
Severity: minor Version: 1.7
Component: Activity Keywords: has-patch
Cc:

Description

When you delete comments the amount in the show all link doesn't change. If you delete and then click the show all, it show's all the comments including the previously deleted comments.

Create 7 comments
Refresh page to get "show all X comments" link
Delete a few comments no change to amount
Click the show all link
previously deleted comments reappear

Attachments (1)

4844.patch (1.2 KB) - added by merty 7 years ago.

Download all attachments as: .zip

Change History (10)

#1 @boonebgorges
8 years ago

  • Keywords reporter-feedback added

Can you be more specific about what you mean by "comments"? Do you mean activity replies? If so, I can't reproduce the issue - the "x comments" button changes as I'd expect (and also, I don't see a "show all" link).

#2 @modemlooper
8 years ago

http://dev.taptappress.com/admin/activity/639/

You can see the show all. when you are logged in it says show all 6 comments. If you delete a comment the number doesn't change and if you delete and then click the show all link it shows the previously deleted comments.

#4 @boonebgorges
8 years ago

  • Component changed from Core to Activity
  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 1.8
  • Priority changed from normal to low
  • Severity changed from normal to minor

Thanks for the additional details. I've confirmed both issues. Neither one is a regression; the same problems appear in the 1.6 branch.

#5 @boonebgorges
8 years ago

Marked #3942 as duplicate.

@merty
7 years ago

#6 @merty
7 years ago

  • Keywords has-patch added; needs-patch removed

fadeOut just sets the item's display property to none and leaves it in the DOM. Added a callback to it to remove it from the DOM after the effect is completed.

I don't think updating the X in "Show all X comments" is worth the trouble. I think it's pretty understandable from the user's point of view.

#7 @boonebgorges
7 years ago

In 7000:

When activity comments are deleted via AJAX, remove them from the DOM entirely

Previously, deleted comments were merely hidden using jQuery.fadeOut(). As a
result, when the show/hide comment button was toggled - applying $.show() to
all comment items - the comments would appear again, even though they had been
deleted from the server. Removing them from the DOM ensures that this won't
happen.

See #4844

Props merty

#8 @boonebgorges
7 years ago

I don't think updating the X in "Show all X comments" is worth the trouble. I think it's pretty understandable from the user's point of view.

I might've been tempted to agree with this. But then I looked at the way this string is currently generated, and was so repulsed that I knew it had to be rewritten :) Basically, we were piecing it together like this: "Show all" + " " + comment_count + " " + "comments". Beyond being breathtakingly ugly, this is extremely untranslatable.

I've refactored the way that the show-all text is generated, so that (a) it's possible to translate it, and (b) it's easy to write javascript to update the number after deletion. Win-win!

#9 @boonebgorges
7 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 7002:

Update 'Show all x comments' count after creating or deleting activity comments

On activity permalink pages, the comment list is collapsed if the item has more
than 6 responses, and a link is appended that reads 'Show all x comments'. This
changeset introduces additional JavaScript to update this count when a new
activity comment is created, or an existing one is deleted, via AJAX.

In the process of making the change, the logic for constructing the 'Show all
x comments' string was changed, so that it's now properly translatable.

Also cached a JS variable that was being used many times, to avoid unnecessary
DOM traversal.

Fixes #4844

Note: See TracTickets for help on using tickets.