Fixed IE10/11 support (#34) #35

jhildenbiddle · 2018-02-14T00:25:54Z

Changed NodeList-to-Array conversion from spread operator to Array.apply
Changed Array.includes instances with Array.indexOf
Changed ES6 code in demo.js to ES5 as this code is not transpiled via babel
Added CustomEvent polyfill and converted dispatched Event() instances to CustomEvent()

- Replaced NodeList-to-Array conversion from spread operator to Array.apply - Replaced Array.includes instances with Array.indexOf - Added CustomEvent polyfill and converted dispatched Event() instances to CustomEvent() - Converted ES6 code in demo.js to ES5 as this code is not transpiled via babel

francoischalifour

That's awesome, thank you very much! I just have a small concern about the polyfill being included in the source code.

francoischalifour · 2018-02-16T15:57:46Z

src/medium-zoom.js

@@ -13,6 +15,23 @@ const isListOrCollection = selector =>

 const isNode = selector => selector && selector.nodeType === 1

+/**


Can we not include the polyfill in the source code? Perhaps document in the README to include this polyfill before including medium-zoom for IE support. What do you think?

Yep. Agreed on not including the polyfill as-is. Libs shouldn't mess with globals (that's my bad).

If it were my choice, I'd switch from a CustomEvent polyfill (which modifies the window object) to a ponyyfill. The rational for this is so that libraries that depend on medium-zoom aren't forced to polyfill to provide support for older browsers. If medium-zoom requires ponyfills to function, then so do all libraries that depend on it (unless they use a forked version of medium-zoom and do the ponyfilling themselves but... that kinda stinks). In this case, we can switch the current CurrentEvent polyfill to a ~6-line ponyfill very easily and prevent devs from having to deal the issue which seems like the right thing to do.

For template support, I did a quick scan of the code and it looks like we could get this working with minimal changes. I haven't spend enough time looking into this to say definitively, but I am happy to do so if you feel this is work exploring. It's worth noting that template support is unavailable for Edge 12, and all versions of Edge have known issues that may effect medium-zoom depending on how users define their template (again, haven't investigated). We can look into these if/when we tackle template support changes and potentially avoid these Edge-specific known issues at the same time. For now, perhaps it is best to handle template support (and any related discussions) as a separate issue and close on these changes which provide "core" support for IE/Edge 12+13 and prevent legacy browsers from breaking.

So, how do you feel about proceeding as follows:

Switch CustomEvent polyfill used in PR to a ponyfill

Add feature detection to allow options.template to be ignored in browsers that do not support HTML templates.

Update README to include browser support section

List support beginning with IE10+

List polyfill requirement for IE and Edge 12 for options.template support

Update README options.support section to include IE/Edge12 polyfill requirement

If this works for you, I can tackle these changes early next week.

Thanks!

That sounds great, thanks for the time you put into this! I'm away next week but ping me if there's anything I can help with.

You bet! Thanks for being amenable to the changes. :)

I'll make the updates mid next week. We can sync up again after the PR request is submitted.

francoischalifour · 2018-02-16T16:10:38Z

As for template support, we can link towards one of these solutions:

I don't have the tools to test these solutions on IE unfortunately.

jhildenbiddle · 2018-02-16T20:38:10Z

If interested, head over to https://www.browserstack.com and sign up for a free open-source developer account. This will make testing in IE much, much easier. The Chrome extension makes any OS/browser combination just a few clicks away.

You can also get a free open-source account on http://saucelabs.com. Same general idea. Both offer live testing in your browser as well as automated testing. I use both (SauceLabs for automated testing, BrowserStack for live testing).

Hope this helps!

francoischalifour · 2018-02-16T21:44:10Z

Thanks for all the tips, that will definitely be useful!

jhildenbiddle · 2018-03-04T09:14:11Z

Apologies for the delay on the update.

The polyfill that was previously included has been removed. In its place is a custom ZoomEvent function which does essentially the same thing (creates modern+legacy compatible CustomEvents) without modifying the window object.

Preview

I did not dive into <template> support, but I can confirm that the combination of this PR + template-element-polyfill allowed the demo, dropbox-paper-template, and facebook-template pages in the repo to function properly on IE10/11. It would be nice if template support for IE was provided without the need for a polyfill, but that was beyond the scope of this PR.

If this PR is merged, I will make sure the maintainers of docsify are notified so that they can update their zoom-image plugin.

Thanks!

francoischalifour

This is brilliant, thanks a bunch!

francoischalifour · 2018-03-04T17:12:12Z

src/medium-zoom.js

+    if (typeof window.CustomEvent === 'function') {
+      return new CustomEvent(event, params)
+    } else {
+      const evt = document.createEvent('CustomEvent')


Can you name this variable something like customEvent?

Yep. :)

Do you prefer customEvent or createCustomEvent? The latter feels like a better match to the existing [verb][Noun] method names. Happy to go whichever way you prefer.

customEvent makes more sense to me since it's an object, not a method. Perhaps customEventFactory is appropriate here?

francoischalifour

Sorry, there was a misunderstanding. I meant renaming the variable evt to customEvent.

But having renamed ZoomEvent to customEvent is also good. I understand why you wanted to call it createCustomEvent.

Now if you can rename customEvent to createCustomEvent and evt to customEvent it would be perfect 🙂

Sorry for the confusion.

jhildenbiddle · 2018-03-04T19:01:20Z

No worries at all. Happy to land this. :)

francoischalifour

Awesome!

francoischalifour · 2018-03-04T19:45:02Z

We need to add the "Support" section in the README and then we're good to release a new version of the library! I'll do that by the end of next week. Thanks a lot!

jhildenbiddle mentioned this pull request Feb 14, 2018

Throws error in IE 11 (Windows 10) #34

Closed

francoischalifour reviewed Feb 16, 2018

View reviewed changes

Converted CustomEvent polyfill to ZoomEvent

392b079

jhildenbiddle mentioned this pull request Mar 4, 2018

Add compatibility table to readme #36

Merged

Moved ZoomEvent (following existing pattern)

59158ed

francoischalifour reviewed Mar 4, 2018

View reviewed changes

Rename ZoomEvent to customEvent

9ea9c51

francoischalifour reviewed Mar 4, 2018

View reviewed changes

Renamed customEvent to createCustomEvent

b506b1e

francoischalifour approved these changes Mar 4, 2018

View reviewed changes

francoischalifour merged commit bd485fc into francoischalifour:master Mar 4, 2018

jhildenbiddle mentioned this pull request Mar 10, 2018

Update medium-zoom version to 0.4.0 (IE10/11 compatibility) docsifyjs/docsify#417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed IE10/11 support (#34) #35

Fixed IE10/11 support (#34) #35

jhildenbiddle commented Feb 14, 2018

francoischalifour left a comment

francoischalifour Feb 16, 2018

jhildenbiddle Feb 16, 2018 •

edited

Loading

francoischalifour Feb 16, 2018

jhildenbiddle Feb 16, 2018

francoischalifour commented Feb 16, 2018

jhildenbiddle commented Feb 16, 2018

francoischalifour commented Feb 16, 2018

jhildenbiddle commented Mar 4, 2018 •

edited

Loading

francoischalifour left a comment

francoischalifour Mar 4, 2018

jhildenbiddle Mar 4, 2018

francoischalifour Mar 4, 2018

jhildenbiddle Mar 4, 2018

francoischalifour left a comment

jhildenbiddle commented Mar 4, 2018

francoischalifour left a comment

francoischalifour commented Mar 4, 2018

		@@ -13,6 +15,23 @@ const isListOrCollection = selector =>

		const isNode = selector => selector && selector.nodeType === 1

		/**

Fixed IE10/11 support (#34) #35

Fixed IE10/11 support (#34) #35

Conversation

jhildenbiddle commented Feb 14, 2018

francoischalifour left a comment

Choose a reason for hiding this comment

francoischalifour Feb 16, 2018

Choose a reason for hiding this comment

jhildenbiddle Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

francoischalifour Feb 16, 2018

Choose a reason for hiding this comment

jhildenbiddle Feb 16, 2018

Choose a reason for hiding this comment

francoischalifour commented Feb 16, 2018

jhildenbiddle commented Feb 16, 2018

francoischalifour commented Feb 16, 2018

jhildenbiddle commented Mar 4, 2018 • edited Loading

francoischalifour left a comment

Choose a reason for hiding this comment

francoischalifour Mar 4, 2018

Choose a reason for hiding this comment

jhildenbiddle Mar 4, 2018

Choose a reason for hiding this comment

francoischalifour Mar 4, 2018

Choose a reason for hiding this comment

jhildenbiddle Mar 4, 2018

Choose a reason for hiding this comment

francoischalifour left a comment

Choose a reason for hiding this comment

jhildenbiddle commented Mar 4, 2018

francoischalifour left a comment

Choose a reason for hiding this comment

francoischalifour commented Mar 4, 2018

jhildenbiddle Feb 16, 2018 •

edited

Loading

jhildenbiddle commented Mar 4, 2018 •

edited

Loading