search

anselmh / object-fit

Polyfill (mostly IE) for CSS object-fit property to fill-in/fit-in images into containers.
MIT License
996 stars 92 forks source link

Use external polyfills #30

Closed TrySound closed 9 years ago

TrySound commented 9 years ago

You can use external polyfill of requestAnimationFrame. It's a little smaller. And dependencies let the code be smaller.

TrySound commented 9 years ago

closest is good for elements.matches.

TrySound commented 9 years ago

For "copy-lib" you may build bundle. For package managers build just core file and maybe getMatchedCSSRules polyfill. Other polyfills will be loaded as dependencies.

TrySound commented 9 years ago

Using closest will a little increase performance.

anselmh commented 9 years ago

Hi, thanks for the suggestions. I agree this could be done and have replaced the rAF polyfill with the one from npm.

I’ll see if I can make use of the closest polyfill and if it makes sense to use it here. Thanks for the hint.

For "copy-lib" you may build bundle. For package managers build just core file and maybe getMatchedCSSRules polyfill. Other polyfills will be loaded as dependencies.

Not sure what you mean here though? Can you explain this a bit more in detail perhaps?

TrySound commented 9 years ago

I meant cdn libs or copying directly in project folder.

TrySound commented 9 years ago

Also add rAF to bower, please.

anselmh commented 9 years ago

Thanks so far for your suggestions. It seems the polyfill is indeed a bit faster now again. I’ll see if I can make the dependency stuff a bit less messy.

TrySound commented 9 years ago

@anselmh Are you sure you need el.closest, not el.matches? closest polyfills both.

TrySound commented 9 years ago

And rAF is not devDep

TrySound commented 9 years ago

Closest is not devDependency too.

TrySound commented 9 years ago

Nice! I asked Jonathan Neal to publish closest on npm.

TrySound commented 9 years ago

@anselmh Closest added to npm https://www.npmjs.com/package/element-closest

TrySound commented 9 years ago

@anselmh Are you sure you need el.closest instead of el.matches? closest polyfills both.

anselmh commented 9 years ago

Thanks. Fixed to use the npm version now. Therefore closing this issue for now.

@anselmh Are you sure you need el.closest instead of el.matches? closest polyfills both.

I tested this and it didn’t fail while speeding up the polyfill. That’s why. If there’s any issue with it it’s easy to roll back.

TrySound commented 9 years ago

@anselmh It works. But el.closest do some more work. It checks current or finds closest parent element with this selector. Better use el.matches.

TrySound commented 9 years ago

@anselmh Prefomance, heh))