Skip to:
Content

BuddyPress.org

Opened 12 years ago

Closed 12 years ago

Last modified 8 years ago

#4332 closed defect (bug) (fixed)

Trouble with ajax favoriting activities if different than activity_updates

Reported by: imath's profile imath Owned by:
Milestone: 1.6 Priority: normal
Severity: normal Version: 1.6
Component: Templates Keywords: commit
Cc:

Description

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)

Attachments (3)

bp-activity-template.diff (612 bytes) - added by imath 12 years ago.
global.diff (619 bytes) - added by imath 12 years ago.
global-option2.diff (620 bytes) - added by imath 12 years ago.

Download all attachments as: .zip

Change History (13)

@imath
12 years ago

#1 follow-up: @boonebgorges
12 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 1.6

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

#2 in reply to: ↑ 1 @imath
12 years ago

Hi Boone, i'll do that ;)

#3 @DJPaul
12 years ago

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.

#4 @DJPaul
12 years ago

  • Keywords commit added; dev-feedback removed

#5 @boonebgorges
12 years ago

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

#6 @boonebgorges
12 years ago

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

(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

#7 @johnjamesjacoby
12 years ago

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.

#8 @boonebgorges
12 years ago

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.

#9 @johnjamesjacoby
9 years ago

  • Version changed from 1.6-beta to 1.6

#10 @DJPaul
8 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.