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

Style checks force code to go polymorphic or worse #64

Closed
esprehn opened this issue Mar 26, 2015 · 9 comments
Closed

Style checks force code to go polymorphic or worse #64

esprehn opened this issue Mar 26, 2015 · 9 comments

Comments

@esprehn
Copy link

esprehn commented Mar 26, 2015

width/height/top/left use isDimDefined and isPosDefined which checks for undefined. margin and padding properties use the "in" operator which isn't fast and also means you need to totally omit those properties from your object if you want them to have "no value" for layout.

ex.

if (pos in node.style) {
    return node.style[pos];
}
return 0;

should just be return node.style[pos] || 0;

For getSpacing it should do:

return node.style[key] || node.style[type + suffix] || 0;

This means your style object can have that property all the time and just use the value undefined to get the default. The current code forces you to have many different "types" of Style objects which makes v8 and other JS engines go slow.

@vjeux
Copy link
Contributor

vjeux commented Jun 4, 2015

Totally! The js version has not been written with performance in mind. If you send a pull request i'd gladly accept it :)

@doomb0t
Copy link

doomb0t commented Jun 28, 2015

Hey!
Noticed that this has been sitting for a while and would like to go ahead and implement these optimizations. I can get started right away but want to make sure that @esprehn didn't already tackle this so as to avoid redundancy. I am a CS student who couldn't find an internship this summer so I saved up money to take the summer off and do as much work on open source projects as I can handle. :)

@vjeux
Copy link
Contributor

vjeux commented Jun 28, 2015

Please send out pull requests :)

@doomb0t
Copy link

doomb0t commented Jun 28, 2015

👍

@jaredly
Copy link
Contributor

jaredly commented Aug 5, 2015

@JonathonSonesen were you going to PR with those fixes?

@doomb0t
Copy link

doomb0t commented Aug 5, 2015

Actually you can see my changes at PR #94 but they were wrong, I haven't really had time to work on it but finals are next week then I'll have about five weeks free at that point I might look at it some more at that time.

@jaredly
Copy link
Contributor

jaredly commented Aug 6, 2015

Ok cool :) good luck with finals

@doomb0t
Copy link

doomb0t commented Aug 6, 2015

Thanks man! I plan to slay them, with the sweet knowledge blades I've been forging ;)

@emilsjolander
Copy link
Contributor

Js version is gone, closing this.

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

5 participants