Trouble with ajax favoriting activities if different than activity_updates

[imath]

Hi,

I've noticed that bp-themes/bp-default/_inc/global.js has changed his way to 'ajax favorite' the activities. Before 1.6beta, we had at line 173 target.parent().parent().parent(); now we have at line 191 : target.closest('.activity_update');

The trouble with this change is that if the activity type is not an activity update, then we dont have the class '.activity_update' and then when clicking, it triggers a javascript error and the activity is favorited but not in an ajax way.

Example : '.new_forum_topic' is not '.activity_update' so it leads to an error when using target.closest('.activity_update').

So i tried these 2 options to make the ajax favoriting working :

1/ Modifying /bp-activity/bp-activity-template.php and global.js (attached files bp-activity-template.diff and global.diff)
2/ Simply modifying global.js to come back to target.parent().parent().parent();
(attached file global-option2.diff)

[boonebgorges]

Good find.

The change away from the parent() walker was made so that the bp-default javascript would be a bit more forgiving of modifications in the structure of child theme templates. See #3821, r5998. So I would like to avoid switching back to this method.

In theory, I prefer your suggested method (1) - add a generic activity-item class to *all* activity items, and then look for the closest one of those. This would provide the most flexibility going forward. However, this has the possibility of breaking child themes, at least those that are bypassing bp_activity_css_class() and rolling their own classes. That said, I'm certain that only a very small number of themes fall into this category, and anyway, even if the AJAX breaks, favoriting still works properly (it submits a form request and reloads the page). So if it breaks in a child theme, it will do so pretty gracefully.

Can I get feedback from another core dev?

[Side note for imath: Thanks for attaching diff files. In the future, you can make it even easier for us if you create your diff from the root of your BP plugin directory, which should capture changes in the entire BP codebase in a single diff.]

[imath]

Hi Boone, i'll do that ;)

[DJPaul]

On one hand, I'd prefer a simple revert. But since we've had bp_activity_css_class() in place in DTheme all the way back to at least 1.2, I think we could go with the add-a-generic-class fix. We probably should create a BP Theme Standards document on the codex and record this as a requirement. It would also help the Theme Review Team review BuddyPress themes going forwards as we document the requirements.

[boonebgorges]

Thanks for the feedback, DJPaul. I'm going to go with the selector fix that imath suggested.

[boonebgorges]

(In [6172]) Adds a generic 'activity-item' class to bp_activity_css_class()

Having no class that was shared by all and only activity items made it
difficult to write JavaScript (as for, example, the Favorite and Remove
Favorite buttons) that would select in an elegant way that was not dependent
on HTML element names.

Fixes #4332

Props imath

[johnjamesjacoby]

Reminder: Template Pack will need this, too.

Also, this fix won't be backwards compatible with existing template packers, so we'll need to emphasize theme changes in release notes.

[boonebgorges]

johnjamesjacoby - Thanks for the reminder about BPTP. In this case, it shouldn't affect anything. The PHP change is not at the template level; as long as templates use bp_activity_css_class() (which BPTP templates do), they'll get the new class name. For the past year or so, BPTP has been set up to load its javascript directly from bp-default, which means that it will automatically inherit the JS-level change.

The only potential problems are where someone has modified their template so as not to use bp_activity_css_class(). But this would probably cause lots of JS problems beyond what's described here, and there's only so much we can do to support non-standard markup with bp-default JS.

But you're very correct that we should emphasize this kind of change in release notes, especially for people rolling very customized themes that are based on bp-default.