Move activity reply HTML to template.

[r-a-y]

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)

[DJPaul]

Good catch

[hnla]

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.

[DJPaul]

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

[hnla]

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 :)

[boonebgorges]

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.)

[boonebgorges]

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.

[DJPaul]

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

[Dennissmolek]

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

[Dennissmolek]

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.

[DJPaul]

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

[boonebgorges]

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.

[Dennissmolek]

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.

[r-a-y]

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.

[r-a-y]

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

Attached patch fixes this ticket!

[djpaul]

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

[DJPaul]

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).

[Dennissmolek]

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.

[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. 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.

[Dennissmolek]

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.

[hnla]

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!'

[hnla]

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

[DJPaul]

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

[boonebgorges]

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.

[Dennissmolek]

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.

[DJPaul]

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

[boonebgorges]

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?

[DJPaul]

Sounds like a plan

[boonebgorges]

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

[boonebgorges]

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

[hnla]

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