Issue-688 - replaced classList shim with polyfill #690
Conversation
Matthematic
requested review from
bjankord,
mjhenkes,
ryanthemanuel and
tbiethman
as code owners
| @@ -5,6 +5,7 @@ import KeyCode from 'keycode-js'; | |||
| import onClickOutside from 'react-onclickoutside'; | |||
| import ResizeObserver from 'resize-observer-polyfill'; | |||
| import styles from './HookshotContent.module.scss'; | |||
| import 'classlist-polyfill'; // Full polyfill for browsers with no classList support | |||
There was a problem hiding this comment.
Shouldn't we check if the polyfill is already included before importing it?
There was a problem hiding this comment.
The polyfill handles checking if classList is already defined before doing anything, and i thought webpack took care of bundling repeated imports.
There was a problem hiding this comment.
True, webpack will take care of bundling.
I hate to be a pain, but since we are adding a new third party dependency to our library, I am wondering if we've scoped out the impact. Why did you choose classlist-polyfill over classList.js? Classlist-poyfill mentions they fork classList.js. Does classlist-polyfill have similar issues to whats been logged in classList.js?
Additionally, why did we explore adding the dom4 polyfill suggested by react-outsideclick (the reason we have this shim) instead of classlist-polyfill?
There was a problem hiding this comment.
classlist-polyfill is simply a published fork of classList.js. The owner of classList.js chose not to release on npm, but they are the exact same script, and the owner suggests this package themselves. And the reason for not using dom4 was to try and change and little as necessary since dom4 includes polyfills for all sorts of things in addition to classList. And if it means anything, dom4 is also less popular on npm as far as weekly downloads go.
|
@bjankord It says merging is blocked for me since I'm not an authorized user. Are you able to merge this? |
|
@Matthematic Yeah, this looks good to merge. Thanks for the contribution @Matthematic! 👍 |
Summary
fixes #688 Replaced the classList shim in hookshot with classlist-polyfill