Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance concerns #8

Open
PitPik opened this issue Mar 30, 2016 · 0 comments
Open

Performance concerns #8

PitPik opened this issue Mar 30, 2016 · 0 comments

Comments

@PitPik
Copy link

PitPik commented Mar 30, 2016

Hi Simon,
great idea to do this shim, although I have some performance doubts,...
If you don't want to take care about the following points, just close this issue ;o)

So, you make a jQuery plugin that iterates through a lot of elements...
Every time you make a $('.selector').objectFit('contain'); you actually call doObjectFit that iterates through all elements found by the selector. Good so far, but...
with every call you check if object-fit is supported. This is not really needed. If you check it once, it's enough.

var testForObjectFit = (function() {
    ...
})();

You should just wrap your function in a self invoking wrapper and it gets called the first time it appears and that's it. Inside your doObjectFit function you don't need var supportsObjectFit = testForObjectFit(); any more but only use testForObjectFit as a variable or you can say var supportsObjectFit = testForObjectFit;

Then you have a return this.each(function() {... inside doObjectFit that then calls doResize(this, type); but you define 2 functions inside this function wich is not a good idea inside a for loop (which each actually is). So you should define function findParentRatio outside doResize, ...

...then you create an image every time the function gets called, with a separate onload function. An images load callback gets called every time you do a image.attr("src", $this.attr("src"));, so you can also keep this whole thing outside doResize wich makes the whole function way more performant and use less memory. So, define the image outside and the onload function also. If you see a problem doing this because of the variables type, hideOverflow, $this, ratio and parent then just define them outside the scope of doResize.

The last concern would be $(window).resize(function() {... wich doesn't make sense to check if objectFit is supported, so you better only install this resize callback if testForObjectFit is false.
I would probably also not use the for(var i=0, len = toResize.length; i<len; i++) {... loop to set a timeout but to call doResize so

    $(window).resize(function() {
        clearTimeout(resizeTimer);
        resizeTimer = setTimeout(function() { 
            for(var i=0, len = toResize.length; i<len; i++) {
                var a = toResize[i];
                doResize(a.elem, a.params);
            }
        }, 100);
    });

...not to mention that your for loop would overwrite resizeTimer for every iteration and your first calls never get cleared... and your var a = toResize[i]; also gets overwritten, so you always only call doResize for the last element in the loop.

Good luck with you plugin and keep the spirit going.
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant