Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.If we run toggle class and remove a class that is between two other classes, the remaining classes have no space between them. Example:
elt.className = 'this that another';
toggleClass(elt., 'that');
alert(elt.className);
New class name wil be 'thisanother'.
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | drupal.js_12.patch | 714 bytes | kkaefer |
| #21 | drupal.js_11.patch | 656 bytes | kkaefer |
| #19 | drupal.js_10.patch | 667 bytes | kkaefer |
| #17 | removeClass.txt | 772 bytes | nedjo |
| #14 | drupal.js_9.patch | 582 bytes | kkaefer |
Comments
Comment #1
nedjoThe issue I guess is in removeClass. We use a regular expression that removes the class and any space before or after it. This handles the case that the class is the only one set, that it is at the end, or that it is at the beginning, but not if it is in the middle and one of the enclosing spaces needs to be kept.
Would we do better to handle all our class setting using arrays? We could e.g. split the existing names into an array and then use splice to remove elements.
Comment #2
nedjoHere's a patch that uses arrays to handle class names. We introduce a new Array method, indexOf, that works in a parallel way to the String object's indexOf method: it searches the array for a particular value, returning the index if found and -1 if not.
addClass and removeClass then convert the node.className into an array and call indexOf, adding or removing elements accordingly.
Comment #3
markus_petrux CreditAttribution: markus_petrux commentedExtending a builtin javascript object could conflict with 3rd party libraries, which is very difficult to debug when that happens.
Maybe you could implement the same feature using a standalone function?
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedperhaps a prefixing the new method with drupal_ would suffice?
Comment #5
markus_petrux CreditAttribution: markus_petrux commentedMaybe, but still, something unexpected may happen when extending builtin javascript objects.
This is an example (related to a bug in MSIE):
http://drupal.org/node/47557
I think it's equally easy to create a standalone function that takes an array as its first argument and whatever else it may need. Javascript arguments are passed by reference so this shouldn't be a restriction.
Javascript doesn't have namespaces so when there are chances to mix several libraries (which may happen with Drupal, f.e. with prototype.js or the Yahoo! library, etc.), prefixing global objects with something unique (say drupal_) would be a good idea, however.
Comment #6
nedjoOkay, here's a version with a standalone function rather than prototyping the Array object.
Comment #7
Steven CreditAttribution: Steven commentedWhy do you use the lambda-like function assignment instead of a normal declaration? Don't you need to declare the variable with 'var' then?
I'd prefer the normal function syntax.
Comment #8
nedjoThanks for pointing this out, just a bad edit on my part from my previous patch. Here it is with a normal declaration.
Comment #9
nedjoThis patch raises errors if there is no className set. We need to test for node.className before acting on it. I'll work on fixing this.
Comment #10
nedjoI'm still kinda partial to treating classes as arrays, but here's a much smaller patch.
Instead of refactoring our approach, we change removeClass to test separately for classes with a space before and a space after, so that two spaces won't be removed.
Comment #11
drummLets say we have
class="my-class-with-lots-of-words"and we attempt to remove 'my-class.' I that a chunk of the class name would be removed since there is no test for end of word. There is a way to match end of word in many regexp varients, usually '>' or '\>.'Comment #12
nedjoGood point. Regular expressions have never been my strong point (which probably begs the question, why am I submitting a regex patch? ;) ). Anyway, \b seems to be the Javascript regex word boundary match. Here's a patch that tests for beginning/end of string or word boundary.
Comment #13
Dries CreditAttribution: Dries commentedSee also #64639.
Comment #14
kkaefer CreditAttribution: kkaefer commentedHere is a new patch (which is also different from the one I posted at http://drupal.org/node/64639).
It works with word boundaries and does also handle classes with dashes inside correctly.
Comment #15
kkaefer CreditAttribution: kkaefer commented(sorry, didn't know that changing the title affects the whole issue now)
Comment #16
Dries CreditAttribution: Dries commentedI suggest that we document this change/requirement with a code comment.
If Nedjo can confirm that this patch solves his problem, we can commit it to CVS.
Comment #17
nedjoOn my basic testing this fixes the issue.
Why do we need special handling for dashes (-) ? Are they interpreted as word boundaries?
Here's how I tested the patch (no patching of files required): copy the attached into a 'page' or 'story' node, set input type as PHP. It loads drupal.js, redefines the removeClass() function, and then runs a test, setting a three-part class name and removing the middle part.
Konstantin, can you add a one-line code comment explaining the filtering?
Comment #18
Dries CreditAttribution: Dries commentedWith some documentation, this patch looks ready to go. Thanks Nedjo for testing.
Comment #19
kkaefer CreditAttribution: kkaefer commentedAgain a new patch. I figured out, that my previous patch was way too complex (it worked though) because we can achieve that much simpler. It actually doesn't matter if there is more than one space between two classes so we can safely punch in just another space to be sure that there *is* whitespace. That also solves the word boundary problem because yes, a dash is a word boundary in regex. In my previous patch, that was solved with a lookahead which makes sure that there is no dash directly following. In the new patch however, this isn't needed.
Nedjo: You should've added another class calles 'one' and remove that class without word boundaries. Because then, it also would have removed *every* 'one' in every class.
Comment #20
Dries CreditAttribution: Dries commentedThe code looks clean and elegant, but I can't make sense of the code comment. Not without reading up on this issue anyway. I suggest we work on the code comment some more. Thanks! :)
Comment #21
kkaefer CreditAttribution: kkaefer commentedAnother code comment
Comment #22
nedjoLooks good to go.
Comment #23
Dries CreditAttribution: Dries commentedSorry but the code comment is not entirely clear yet. It explains what is going on, but not why.
Comment #24
kkaefer CreditAttribution: kkaefer commentedAdded the reason for the change to the code comment.
Comment #25
Dries CreditAttribution: Dries commentedExcellent. Comitted to CVS HEAD. Thanks for the persistence Konstantin.
Comment #26
Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commentedalso to 4.7.
Comment #27
(not verified) CreditAttribution: commented