tmort / Socialite

Other
1.68k stars 163 forks source link

Added trim() in removeClass to prevent space creep in el.className #23

Closed IDisposable closed 12 years ago

IDisposable commented 12 years ago

Added trim() in removeClass to handle the case when it is called against a non-empty el.className set that does NOT include the to-be-removed class. Without this, spaces will be left on the front and back of the el.className. Since this is a frameworkless project, added String.prototype.trim if not native.

dbushell commented 12 years ago

Thanks Marc,

This made me notice another bug (just fixed) with removeClass. It should be replacing with a single space not an empty string. Otherwise removing the middle class from a b c results in ac, not a c.

As for adding trim(), while I can see this fixes the imperfection from what I understand the potential spaces generated are harmless.

Since MDN suggests the same polyfill I'm not worried about overriding the prototype, but I'm not convinced we need to add trim() only because it would mean a few extra bytes for no real gain.

Either way, could we not do:

el.className = (' ' + el.className + ' ').replace(' ' + cn + ' ', ' ').replace(/^\s+|\s+$/g,'');

If the rogue spaces were significance? Just a bit shorter.

IDisposable commented 12 years ago

I was nervous about the polyfill approach, but it does seem to be the best practice version and has the advantage of being native almost everywhere (and thus much faster).

As for the spaces being significant, sadly i've seen code in the wild checking for empty className or doing a direct compare instead of a contains. While not good code, I would hate for an unintentional side-effect to break them.

So, I feel we need a trim, and we could not polyfill in which case I would add a local var holding either the native function or the polyfill.

dbushell commented 12 years ago

Ok I'm sold :)

Pull in the bug fixes I did yesterday and do another pull request so I can merge easily. Create socialite.trim() at the start of the file and use native / fallback in there. May be useful elsewhere in future. Then I suppose it'd be:

el.className = socialite.trim((' ' + el.className + ' ').replace(' ' + cn + ' ', ' '));