Forked domtokenlist

[jantimon]

I don't understand why the domtokenlist is a fork. Do you really need the UMD handling?

https://github.com/jwilsson/domtokenlist/compare/master...rodneyrehm:4ccd85f5662e674bd9a7d1a9e121eb49de42a14a?expand=1

We got some firewall issues which do not allow to install directly from github. Do you think we can get your fork merged and use npm again?

[rodneyrehm]

Do you really need the UMD handling?

yes. The tests (using intern) require AMD. The build (using browserify) requires CommonJS. Until there is a decent support for ES6 modules in a test framework like Intern, I fear I'm stuck with this module mess.

We got some firewall issues which do not allow to install directly from github.

interesting.

Do you think we can get your fork merged and use npm again?

My code already landed in the original project, but was reverted before they released 1.2.0. Because of that I kept the fork alive.

As I was generally unhappy with domtokenlist-shim, I went ahead and implemented my own tokenlist. However, I did not want to just swap the implementations, without making use of the additional functionality my own implementation.

The domtokenlist-shim is currently only loaded in focus-source.js and focus-within. We currently only need .classList.add() and .classList.remove().

It is entirely possible to drop the polyfill altogether and use one of the older approaches to this (i.e. do the string manipulation ourselves). I believe this would be the best course of action for now.

---

Are you using the bundle or individual modules? Maybe it would make sense to publish the bundle separately without dependencies, as they're already included anyway. That doesn't help if you're using individual modules, though…

[jantimon]

Okay that's understandable.

Right now we are not using the bundle. The current solution is a small helper ts file which picks directly the focus source file out of ally.js:

/**
 * Allow to distinguish between tab and mouse focus interactions
 * @see http://allyjs.io/api/style/focus-source.html
 *
 * Usage:
 * ```css
 * [data-focus-source='key'] :focus {
 *   background: orange;
 * }
 * ```
 */
const focusSource = <() => any> require('ally.js/style/focus-source.js');
module.exports = focusSource();

[rodneyrehm]

The current solution is a small helper ts file which picks directly the focus source file out of ally.js

Does that mean you were able to work around the problem?

[jantimon]

Only during development as npm install fails on the production machine

[jantimon]

If you like I can prepare a pull request which uses string methods to add/remove the classes

[jantimon]

148 is now ready for review

[rodneyrehm]

Merged! Thanks for your efforts! :)

I imagine you'd like a new release asap. As I'm still not in a position to release the other changes in master, I'll have to create another release based off v1.1.1 only containing this fix. Since we're removing a dependency that had a global effect (DOM prototype), I'm not sure if this should become 1.2.0 rather than 1.1.2…

---

You mentioned somewhere that you weren't able to run the tests locally. What was the problem?

[jantimon]

Yes sure it would be cool to release 1.2.0 soon. It's either a feature release 1.2.0 (works without domtoken plugin) or a breaking change release 2.0.0. (doesn't include domtoken plugin)

Local tests:

[rodneyrehm]

Your contribution is now known as release 1.2.0 - Adios DOMTokenList - thank you for your efforts! :)

[rodneyrehm]

regarding your ability to run tests…

• Have you run npm run init or npm run clean && npm run build before running npm run test?
• Are you by any chance using npm v3?

[jantimon]

Thanks for the release 🎉🎉🎉

Moved the local testing problems into a new issue: #149 However although it is very slow it is also fine to work on a feature branch and let travis run the tests.