Unbind selectors from tokens in jQuery

[hnla]

This patch removes all of the selectors that are bound to their tokens e.g jq('div.item-list-tabs') becomes simply ('.item-list-tabs')

Primary purpose of this change is to facilitate the ability to be able to re-factor markup without breaking JS and more specifically to allow re-factoring of templates to html5 selectors (As in example above where 'div' becomes 'nav')

This, in theory, is a safe change that ought not to cause any issues.

Possible consequences might arise if devs have gone and used the token elsewhere on another element but in reality that would have been an unwise move?

I have changed all instances of selectors to leave only tokens throughout global.js reading through these changes are ok but am still testing across installations to try and spot problems and suggest this patch does require testing in depth before accepting

Notes:

Line 594 nnwards where we build 'bp_filter_request' could be an issue and needs checking carefully; I have removed the div from the selector in the queries in these various blocks and it ought not to present issues.

Interesting to note in some areas, where for instance I removed div from 'div#message', I find, later on, instances where it is written as '#message' suggesting this removal is a safe action especially as an ID may only be used once per page so 'div' is unnecessary albeit formally correct.

[hnla]

Remove element selectors from jquery

[johnjamesjacoby]

Looks good to me. Shouldn't break anything and makes the code more portable.

[r-a-y]

There could be a problem with lines 335 and 1258 of the patch as there's a body class that also uses "activity" when on an activity component page.

[hnla]

hmm didn't get notifications on this ticket.

@jjj Yes I think by and large it feels ok but it's a slightly tricky one to test and we could do with as many people as possible running with the patch to see if any js functionality breaks on their dev installs.

Thanks r-a-y I'll look at those lines.

[hnla]

How does this sound for dealing with lines 335 /1258:

originally:
jq('div.activity').click( function(event) {

my change:
jq('.activity').click( function(event) {

revised to?

jq('body .activity').click( function(event) {

If the issue is that body could contain the class 'activity' and therefore be a problem in that event function then remove the problem by declaring a descendant selector set so .activity now is required to be a child or grandchild element of parent/antecedent element body hopefully preventing the script taking action on body.activity?

[boonebgorges]

This probably won't break anything with bp-default. The real issue is whether it will break child themes who are loading bp-default's JS, or (perhaps more likely) themes built based on the BP Template Pack.

I'm guessing it probably will only in edge cases, though I would probably have to pore over each one of the proposed changes to be able to guess intelligently.

How much do we care about the possibility (slight though it may be) of breaking bp-default's JS with child themes?

[r-a-y]

hnla - The proposed change is better, but I'm still leery of how generic the .activity selector is. Some rogue theme developer might use the "activity" class for whatever reason! :)

boonebgorges - You're right. Classes and IDs like "error" and "message" could be used in WP themes, which could interfere with the JS if BP Template Pack is used.

[hnla]

Yep this is the about the only 'edge case' of concern that these tokens are used elsewhere.

The term 'rogue' is apt though as I would say this would be an unwise and shortsighted thing to do definitely not something I would do if I were using global.js in it's default state in fact to re-use an establish class or id in this manner is just asking for trouble.

Question is, and as asked earlier by Boone, how much do we care about this possible edge case breaking child theme? the simple notion of breaking a child theme is obviously something we would never want to knowingly do but in this instance I am going to say that given they(the rogue dev) ought really not to have done this that we don't accept this as a reason not to go ahead; the benefits to doing this are vast and also I fear if we can't do this then if child themes or bp-default require the present js functionality they will not be able to move to a fully realised HTML5 layout.

I'm still concerned though that there isn't a more genuine use case that would cause, or present, problems not thought of yet.

[boonebgorges]

3821.02.patch is hnla's patch, with the element names reattached to:
.activity
.error

There are two other selectors that are pretty common words that might cause child theme problems (#sidebar and #message), but since both are ids, they should be unique on the page anyway. If they're not, then it's a broader problem of invalid markup.

I've done some testing and these changes look good. If there are no objections, I'll commit this change in a few days.

[hnla]

Just spotted my damned dyslexic error in the patch numbering *sigh*

Cheers for the Revision if only you had waited a few more hours I had penciled in Sat morning to add a revision along those lines :)

I'll run the -02 patch and see how it fares.

[hnla]

@Boone Having trouble applying this patch, I note it says 'Git' in the header is this in fact a Git patch rather than SVN?

[johnjamesjacoby]

(In [5756]) Unbind selectors from tokens in global.js Fixes #3821. Props hnla, boonebgorges.

[hnla]

In light of issue found with favs / unfavs by Boone ​https://github.com/karmatosed/Status/issues/70

re-opening this ticket with possible fix for problem of activity favorite buttons traversing back up the dom by a fixed factor '.parent().parent().parent()' naturally any deviation from the button nested at this level is going to break the functionality.

Suggested fix is to replace .parent() with .closest('li')

At present (line 192):

if ( target.hasClass('fav') || target.hasClass('unfav') ) {
	var type = target.hasClass('fav') ? 'fav' : 'unfav';
	var parent = target.parent().parent().parent();

replaced with:

if ( target.hasClass('fav') || target.hasClass('unfav') ) {
	var type = target.hasClass('fav') ? 'fav' : 'unfav';
	var parent = target.closest('li');//.parent().parent();
	var parent_id = parent.attr('id').substr( 9, parent.attr('id').length );
	alert(parent_id);

Alert() returns correct id and in testing this appears to be a workable change - also .closest has far less overhead than .parent() which has to traverse fully through the DOM to top node and retain data whereas .closet() halts on first match.

There may well be issues I've not spotted though and this in this form is still limiting in presuming to be able to find a li element but then a list is the correct markup for an activity list.

P.S This does not appear to be a fix necessarily for Status theme as that seems to have other issues brought to light and needs further investigating.

[boonebgorges]

hnla - The change to .closest() is smart. However, looking for li rather than a selector seems to reproduce some of the problems this ticket was originally made to solve (ie, the inability to change the markup to something more HTML5ish). I'm going to use .activity_update instead.

[boonebgorges]

(In [5998]) Switches parent() walker to .closest() when looking for activity parent item in activity-favoriting JS, for better child theme compatibility. Fixes #3821. Props hnla

[hnla]

LI is a selector ;) but yes it troubled me although that said anyone thinking they can improve on markup structure and not use a ul/ol construct is barking mad :) (dl/dd excepted)

The class token is good though, we essentially say if you want the scripting functionality please ensure specific required tokens are used /preserved which is more than acceptable to expect and ask.

I'm now thinking I/we need to read through this file in a little more detail for any further occurrences such as the .parent().parent().parent() restriction.

[hnla]

Missed a 'div' on #new-topic-post lines 12,14

Patch 3821.03 removes 'div' from selectors

[hnla]

remove 'div' from '#new-topic-post'

[boonebgorges]

(In [6033]) More unbinding of JS actions from overspecific selectors

This changeset removes the 'div' specifier from the bp-default js that shows/hides the #new-topic-post div.

Fixes #3821
Props hnla