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

Make cleaner setCoords and geometry code #1882

Merged
merged 1 commit into from
Feb 2, 2015
Merged

Make cleaner setCoords and geometry code #1882

merged 1 commit into from
Feb 2, 2015

Conversation

asturur
Copy link
Member

@asturur asturur commented Dec 4, 2014

@knagax This PR is mainly about code clean up.
3 methods have been shrunk. Code duplication has been removed and setCoords mad ligther.
Just 4 corners are calculated using the transformation with vpt the other are find "interpolating"
mtr before was a copy of mt , and then made correct in setCornerCoords, now is calculated correct and _setCornerCoords is made simpler.

@asturur
Copy link
Member Author

asturur commented Dec 7, 2014

@kangax Lots of error come out since i moved mtr calculation in setCoords method.
So i have to find out what has screwed up.
Can you get an idea if you like the changes meanwhile?

ok it was related to _calBounds method in group.class.js

Did you ever thought that controls made by circle would made a lot of things easier?
For example find target corner and keeping track of 4 point for each control.

@asturur
Copy link
Member Author

asturur commented Dec 8, 2014

@kangax i'm done. check if you are interested in this.

},

_calculateCurrentDimensions: function(transform) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldTransform is better, to indicate that it's boolean

@kangax
Copy link
Member

kangax commented Dec 8, 2014

@asturur LGTM. Just please please make sure that everything still works — all the demos, front page of fabricjs.com, grouping, iText, etc.

@asturur
Copy link
Member Author

asturur commented Dec 8, 2014

i have first to clone the repo and install ruby.
no hurry to pull this. i will inform you

@kangax
Copy link
Member

kangax commented Jan 28, 2015

Is this good to go?

@kangax
Copy link
Member

kangax commented Jan 28, 2015

/cc @asturur

@asturur
Copy link
Member Author

asturur commented Jan 28, 2015

I did not do all that test suite.
I do not have a mirrored copy of fabricjs.com
Trying in next days
(anyway normal testing, playing in kitchsink of course feels fine)

@kangax
Copy link
Member

kangax commented Jan 28, 2015

Ok

@asturur
Copy link
Member Author

asturur commented Feb 2, 2015

This code looks to no harm to me, i did check trought all the tests and they look visually the same.,

kangax added a commit that referenced this pull request Feb 2, 2015
Make cleaner setCoords and geometry code
@kangax kangax merged commit 95406e0 into fabricjs:master Feb 2, 2015
@asturur asturur deleted the Clean-lines-code branch February 4, 2015 16:20
Alysander pushed a commit to StileEducation/fabric.js that referenced this pull request Apr 10, 2015
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

Successfully merging this pull request may close these issues.

2 participants