Add classList polyfill for IE classList.toggle('foo', force)
What does this MR do?
EE MR, https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3527
- Add
classList
polyfill for IE,classList.toggle('foo', force)
Are there points in the code the reviewer needs to double check?
Requirements
- Supports second parameter
force
in thetoggle
method,classList.toggle('foo', true)
, see https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Methods - Works with SVGs (
classList
doesn't work at all with SVGs in IE natively) - Lightweight
- Available on
npm
(nice to have)
Options
-
classlist-polyfill
- Available on npm, https://www.npmjs.com/package/classlist-polyfill
- Has tests
- No extra sub-dependency bloat (5.56kb)
- Works with SVGs
- Fork from
eligrey/classList.js
but planned to be deprecated now that the upstream has new maintainers
-
eligrey/classList.js
- Not available on npm (manual dependency management)
❌ - Has tests
- No extra sub-dependency bloat
- Does not work with SVGs
❌
- Not available on npm (manual dependency management)
-
remy/polyfills/classList.js
- Not available on npm (manual dependency management)
❌ - No tests
❌ - No extra sub-dependency bloat
- Does not work with SVGs
❌
- Not available on npm (manual dependency management)
-
devongovett minimal gist
- Not available on npm (manual dependency management)
❌ - No tests
❌ - No extra sub-dependency bloat
- Does not work with SVGs
❌
- Not available on npm (manual dependency management)
Also see https://github.com/Modernizr/Modernizr/wiki/HTML5-Cross-Browser-Polyfills#classlist
Why was this MR needed?
IE 11 does not support force
parameter in classList.toggle('foo', force)
.
We have existing code that just doesn't work correctly and future code,
- https://gitlab.com/gitlab-org/gitlab-ce/blob/50a5d638e56443ffc7d4d08b3706b9c63e372dfa/app/assets/javascripts/label_manager.js#L46
- https://gitlab.com/gitlab-org/gitlab-ce/blob/50a5d638e56443ffc7d4d08b3706b9c63e372dfa/app/assets/javascripts/landing.js#L14
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Tests added for this feature/bug - Review
-
Has been reviewed by Frontend
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Squashed related commits together -
Internationalization required/considered
What are the relevant issue numbers?
Closes #38916 (closed)
Edited by Eric Eastwood