Element className / classList handling #2594

Closed
timwienk opened this Issue May 12, 2014 · 1 comment

Projects

None yet

2 participants

Owner

Reference: #2543

For version 1.5, we decided to keep the undocumented pre-1.5 behaviour of handling mutiple classes in addClass and removeClass. For 1.6 we should decide whether we want to document (and therefore publicise) this behaviour, or whether we want to handle multple classes in a different way (or at all).

CC of Christoph's text describing the issue:

ahm, wait, do we actually want to support multi classes in these methods?

  • We never actively supported it (the fact that the code supports it is a bug).
  • The documentation says it only works with a single class name.
  • The method names are singular: addClass, not addClasses
  • This is noticeably slower for the 99 % case where you only want to add a single class

Before this change this is what's happening:

=> addClass('a b'); // ends up with "a a b"
=> removeClass('a b') // works but removeClass('b a') doesn't

I don't think we want to make assumptions about the order of class names of an attribute of an HTMLElement.

I think it is far more reasonable to either:
a) add checks for the non-classList behavior that throws when calling the method with null, an empty string or an invalid class name
b) only handle the old behavior if you are using the 1.4compat version (in this case it could just do hasClassList = false)

@timwienk timwienk added this to the 1.6.0 milestone May 12, 2014
@timwienk timwienk added a commit to timwienk/mootools-core that referenced this issue May 12, 2014
@timwienk timwienk merge: Use classList in Element where available. (#2543)
This change optimises and fixes certain behaviour in Element's addClass,
hasClass and removeClass methods. The behaviour fixed is an undocumented
way to handle multiple classes through these methods.

The decision was made to not introduce a potential breaking change (even
though undocumented) in this stage of 1.5 development. The undocumented
behaviour may be (re)moved for a next major release.

Closes #2543.
Opens #2594.
d7e5093
Owner
ibolmo commented Sep 4, 2014

Is this not resolved?

@ibolmo ibolmo closed this Sep 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment