Add rollup build process and update README #27
Conversation
|
Thinking about this a bit more... I'm not in love with the idea of forcing everyone to download the polyfills, even though they're quite small. I'm curious what folks think about recommending the use of a service like Polyfill.io or encouraging devs to include those polyfills before using focus-ring if they determine they need to (likely through UA sniffing). |
|
@robdodson I think your PR is fine as is; the stated goals are clear. IMO the "forcing everyone to download the polyfills" question, while considerate of you, is an overthink. My bias would be towards the fastest path towards getting this up for people to use in their projects—that'll give us the most valuable feedback. As you said, "we could create a separate build for those platforms that strips these dependencies" should that come up. Lastly, regarding providing a minified version—not likely something to worry about for this PR either as those pulling this in are likely going to be using it as part of a project where they have their own build step that will handle minification . |
|
OK I'm cool with that. Definitely happy to keep making forward progress and we can find out how to slim things down in the future. |
|
@robdodson any update as to when this branch will be merged? Additionally, when can we plan on this being published to NPM? |
|
@robdodson Slick! Things are looking great!. I agree w/ @kloots on all fronts. In my testing IE9 chokes on event listeners due to missing doctype in demo. |
|
Just waiting on @alice for the review. We'll be able to get this up on npm in the next couple of days |
README.md
Outdated
| pseudo-selector, | ||
| this prototype adds a `focus-ring` class to the focused element, | ||
| in situations in which the `:focus-ring` pseudo-selector should match. | ||
| [`:focus-ring`](https://drafts.csswg.org/selectors-4/#the-focusring-pseudo) pseudo-selector, this |
There was a problem hiding this comment.
Please don't reflow text: http://rhodesmill.org/brandon/2012/one-sentence-per-line/
| // These elements should always have a focus ring drawn, because they are | ||
| // associated with switching to a keyboard modality. | ||
| var keyboardModalityWhitelist = [ | ||
| 'input:not([type])', |
|
@kloots we published focus-ring to npm as wicg-focus-ring. Let me know if you hit any issues and thanks for giving it a go! |
This is a big PR that I hope addresses both #24 and #18.
The gist of this PR is that it switches the project over to using ES Modules and produces a UMD build using rollup. This build should be consumable by module loading tools like Browserify, or it can be used standalone as a regular
<script>.I've added polyfills for
matchesandclassListthat should enable support for older versions of IE and Safari. This increases the overall LOC for the project but looking at the polyfill implementations, I think they're quite similar to what we were already about to add, they just have a ton more white space and comments. Ideally these should all get stripped out when the developer does their final build. I'm also happy to include a minified version of the UMD build if folks think that would be helpful.One hope I have is that by moving over to modules and turning the polyfills into dependencies, we can figure out a way to remove them using tree shaking features of rollup. For example, any browser that supports ES Modules will have
classListandmatchesalready, so we could create a separate build for those platforms that strips these dependencies. I'd like to tackle that after this PR lands.@alice @kloots @joe-watkins could you all please take a look at let me know what you think?