Skip to:
Content

BuddyPress.org

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#3253 closed enhancement (fixed)

Move activity reply HTML to template.

Reported by: r-a-y's profile r-a-y Owned by:
Milestone: 1.5 Priority: normal
Severity: Version: 1.2
Component: Activity Keywords: has-patch
Cc:

Description (last modified by DJPaul)

In trunk, post an activity update and try to reply to the update past 2 levels.

eg.

My update

Reply 1

Reply 1.1 (refresh the page and hit "reply" from here and the activity comment form will not appear)

Reply 2

(EDIT: Move the template out of PHP, into BP-Default)

Attachments (4)

3253.diff (2.0 KB) - added by boonebgorges 14 years ago.
3253.1.diff (10.7 KB) - added by boonebgorges 14 years ago.
3253.02.patch (1.3 KB) - added by r-a-y 14 years ago.
3253.3.diff (15.8 KB) - added by boonebgorges 14 years ago.

Download all attachments as: .zip

Change History (35)

#1 @DJPaul
14 years ago

  • Milestone changed from Awaiting Review to 1.3

Good catch

#2 @hnla
14 years ago

I noticed issues here as well it's a damned Ajax thing, my personal bugbear!

If posting an update I can't actually reply to this update initially without refreshing the page, this is a condition unlikely to arise though and is not exactly what r-a-y reports.

On another 1.3 install both my perceived issue and R-a-y's are not apparent and all works fine?!

Not sure what the difference is here, there's something not right but it's not easily trackable.

#3 @DJPaul
14 years ago

This is because bp_activity_recurse_comments() does not output a reply comment form. Did this work on 1.2?

#4 @hnla
14 years ago

Tested on 1.2.8 and I have the same issue: can't reply to an update I've made without refreshing, however I think this isn't an issue really as I can't think of a case where one wouldn't have refreshed page or left page rather than be trying to reply to ones own entry, so think I'm confusing issues with this one.

However following r-a-y's test case above on 1.2.8 didn't demonstrate the problem as mentioned, but in making several rapid updates and comments the third or fourth update refuses to submit? trying to refresh page, clearing textarea starting over, nothing can't submit, I can however continue adding comments to existing entries, moved off page and back and all is ok??!!

Beginning not to like the feel of this :)

#5 @boonebgorges
14 years ago

I just messed with this for a while. As usual, it's going to be much harder than it seems to make this work. Simply adding a form to bp_activity_recurse_comments() is not enough, because the JavaScript is written in a non recursive way. I tried making this work, but in truth it's likely that the comment reply JS will have to be completely rewritten to make something solid.

In light of this, and in light of the fact that this is not a regression, I want to nominate this issue for punting beyond 1.3. (Unless someone really feels like rewriting the AJAX.)

#6 @boonebgorges
14 years ago

FWIW, here is a patch that will get you up and running with the form markup, if you feel like messing with it. Again: 3253.diff is NOT a working patch - the JS needs to be retooled - but this may save you a few minutes.

@boonebgorges
14 years ago

#7 @DJPaul
14 years ago

  • Priority changed from normal to critical

I've just checked, and it is a regression; we need to fix this for 1.3

#8 @Dennissmolek
14 years ago

So much HTML in that core.. is there anyway to get it to a template file? or is that a later kind of thing?

#9 @Dennissmolek
14 years ago

on testbp if you make the comments they are posted via ajax, then you can keep going and the form shows up each time.

Once you refresh the page, clicking reply nudges the paged down to where the form SHOULD be but does not expand the page as before.

#10 @DJPaul
14 years ago

We discussed this earlier and Boone volunteered to look at moving the HTML into a new template file, and to re-write the javascript.

#11 @boonebgorges
14 years ago

3253.1.diff is a swing at making this work. I broke the activity comment out into its own template, and it seems to be working well.

Could I ask someone to give it a shot? If it works well, I'll write up the (enormous amounts of) PHPDoc necessary. Also, I'll port it over so that the template is responsible for the AJAX return.

@boonebgorges
14 years ago

#12 @Dennissmolek
14 years ago

I figured this out. The form is called from a array created with the comment ID. Say my comment is number 23.
the first comment has the ID:acomment-comment-23 and the second comment has acomment-reply-23. The form is called ac-form-23
The reply button creates an array with a split using the(-) and then uses the second item which in both cases is 23. the form name is aform-23. So it all works.

in any sub replies the replies are added up. In this case, acomment-reply-24, than acomment-reply-25, etc. This calls a form that does not exist. This doesnt happen when you do it in ajax because it gives the ID:acomment-reply-23 to all the reply buttons. Once you refresh the new page it loads it with the adding numbers.

Now two items having the same ID is bad code so I'm working on a patch, but to see the fix just use firebug on any reply button that isnt working, change its id to: acomment-reply- then the comment number of the main comment.

#13 @r-a-y
14 years ago

The problem appears to have occurred during the refactoring of bp_activity_recurse_comments().

I copied over the BP 1.2 version into BP 1.3 and nested comments work again.

---

Regardless, an activity comments template will make many theme devs very happy!

Boone, 3253.1.diff needs a conditional check to use the old activity comments code if BP can't locate the new activity comment template. Otherwise, activity comments will break for existing BP themes.

Last edited 14 years ago by r-a-y (previous) (diff)

#14 @r-a-y
14 years ago

As always, it's the simplest things we overlook! ;)

Attached patch fixes this ticket!

@r-a-y
14 years ago

#15 @r-a-y
14 years ago

  • Component changed from Theme to Activity
  • Keywords has-patch added
Last edited 14 years ago by r-a-y (previous) (diff)

#16 @djpaul
14 years ago

(In [4478]) Fixes nested activity comment reply javascript, props r-a-y. See #3253

#17 @DJPaul
14 years ago

  • Description modified (diff)
  • Priority changed from critical to normal
  • Summary changed from Nested activity comment reply breaks after level 2 to Move activity reply HTML to template.
  • Type changed from defect to enhancement
  • Version changed from 1.3 to 1.2

Excellent work all! I've put in r-a-y's patch, but it makes sense to test and complete Boone's patch so we can take the HTML out of the PHP (espc. as the patch appears to be mostly complete).

#18 @Dennissmolek
14 years ago

Boone's fix is using bp_activity_comment_id() which does the same thing, but pulls the code into an adjustable template file. although .02 works .01 is a better solution.
In my tests its working great!

There is still the issue of the ID being used multiple times. I'm going to work a quick fix for that. Also to note, the Ajax calls a TON of .parent() which is highly theme specific.

Last edited 14 years ago by Dennissmolek (previous) (diff)

#19 @Dennissmolek
14 years ago

I want to also so that while doing all these testing I found it annoying that when replying to comment 2, I have to go past comment 5 to get to the form. changing line 325 of code in global.js to this:
jq('div.activity-comments li#acomment-' + c_id+'>div.acomment-content').after( form );

puts the form box just underneath.

#20 @Dennissmolek
14 years ago

also, a easy fix for the ID issue: change the reply button ID:
id="acomment-reply-<?php bp_activity_id() ?>-from-<?php bp_activity_comment_id() ?>"
(uses Boone's patch)
resulting ID looks like acomment-reply-23-from-32
not a super sleek change but makes sense and keeps ID's different. A better solution would be to adjust the AJAX but this is working.

Last edited 14 years ago by Dennissmolek (previous) (diff)

#21 @hnla
14 years ago

Was interested in testing Boon's patch sadly it doesn't want to merge

error: 'The line indicating the new file was expected in line 219!'

#22 @hnla
14 years ago

On a general note: duplicate ID's have always existed in BP and eventually they ought to be tackled and corrected.

#23 @DJPaul
14 years ago

hnla; Boone's patch will apply against r4477. It needs to be updated for the latest trunk.

#24 @boonebgorges
14 years ago

Thanks for your investigations, all!

Re multiple IDs: If I'm reading the JS correctly, it's *moving* the existing form, not creating a new one. So there aren't really any duplicates.

Dennissmolek:

I want to also so that while doing all these testing I found it annoying that when replying to comment 2, I have to go past comment 5 to get to the form.

The idea here is that when you have activity item A with comments 1, 2, 3, and 4, you should have to go to the end of all of those comments before posting a reply. This seems no different from blog comments.

I was going to move the markup out of ajax.php and use the new template, but that's going to be harder than I thought, because of the way the markup is currently generated (using bp_has_activities() and the regular activity template tags). Some structural changes will have to be made to make it work. I'll open a separate ticket for it once this change has been committed.

r-a-y:

Boone, 3253.1.diff needs a conditional check to use the old activity comments code if BP can't locate the new activity comment template. Otherwise, activity comments will break for existing BP themes.

Since I'm using locate_template(), this is only true for themes that are not children of bp-default. But you're right. So I changed the logic a little, and added a conditional check, so that if the template is not found in the theme (or its parent), it'll be loaded directly out of bp-default. See 3253.3.diff

3253.3.diff also adds PHPDoc.

@boonebgorges
14 years ago

#25 @Dennissmolek
14 years ago

The ID's of the reply buttons are the same for each sub comment. they are all acomment-reply-[comment ID]

so say there are 40 sub comments, there are now 40 reply buttons with the same ID. If you add a dash then the number like I did it makes them have separate ones and it still fires correctly.

I actually prefer replying to the comment itself.. I think I might make a handy plugin to try it, but I think scrolling past them all is annoying, especially when they go off topic or whatever. No worries though.

#26 @DJPaul
14 years ago

Dennissmolek is correct, replies on the same level share the same IDs.

Version 0, edited 14 years ago by DJPaul (next)

#27 @boonebgorges
14 years ago

Dennissmolek is correct, reply buttons on the same level share the same IDs.

This is also the case in BP 1.2 branch. In fact, the reason why this ticket was initially open was because someone (me, I think) changed these IDs on BP trunk so that they would NOT be the same, so that the JS stopped working.

Since it's not a regression, I would like to commit the changes in 3253.3.diff, close this ticket, and open a new ticket for the less severe issue of duplicate button IDs. Objections?

#28 @DJPaul
14 years ago

Sounds like a plan

#29 @boonebgorges
14 years ago

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

(In [4493]) Moves activity reply HTML to a comment.php template. Fixes #3253

#30 @boonebgorges
14 years ago

See #3279 for further discussion of the duplicate ID issue. Dennissmolek, I give your proposed solution there as one possible fix. Patches welcome.

#31 @hnla
14 years ago

Where possible if duplicate ID's can be removed they ought to be, they are a violation of the specs.

Note: See TracTickets for help on using tickets.