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

Allows setting origin to any value between 0(top/left) and 1(bottom/right) #3098

Closed
wants to merge 4 commits into from

Conversation

virror
Copy link
Contributor

@virror virror commented Jul 10, 2016

I have been using this for quite a while at work with our current project and seems to work fine. Sometimes you need to be able to set another origin that just the predefined ones and then this is perfect. No issues with backwards compatibility, just adds on top.

@asturur
Copy link
Member

asturur commented Jul 11, 2016

If is just this change, i m happy to include it. (style needs fixes)
But i see those functions that does direct use of originX and originY using comparing center , left and right:

_resetCurrentTransform
_setLocalMouse
_flipObject

@virror
Copy link
Contributor Author

virror commented Jul 11, 2016

Ok, thank. Will have a look at the style and those 3 functions.

@virror
Copy link
Contributor Author

virror commented Jul 12, 2016

OK, i have fixed style issues and those 3 functions should now support numeric origins as well. Please review and give me a heads up if somethings wrong.

@virror
Copy link
Contributor Author

virror commented Jul 19, 2016

@asturur Ping!

@asturur
Copy link
Member

asturur commented Jul 20, 2016

i watched the code a couple of time and i think made you change that other
functions for no reason.

i need to download it and try, i very busy those days.

On Jul 19, 2016 23:12, "Victor Hagsand" notifications@github.com wrote:

@asturur https://github.com/asturur Ping!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3098 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABI4QH8_Lt7ExEWwMywKN1PNO7pMrsWaks5qXT2hgaJpZM4JI7fh
.

@virror
Copy link
Contributor Author

virror commented Jul 20, 2016

I just did as you said : p
No hurry, just wanted to make sure you forgot about it.

@virror
Copy link
Contributor Author

virror commented Jul 21, 2016

Don't forget, lol

@asturur
Copy link
Member

asturur commented Jul 24, 2016

merged in a temporary branch so i can do tests.

@asturur
Copy link
Member

asturur commented Jul 24, 2016

@virror i had to do some corrections, there were errors.
I would like to pull your work anyway.
If you have time take #3121 and make a merge over your branch of my changes.
Basically:
canvas.class.js needs to have no changes, those functions i asked you to check did not need for changes.
You left some syntax error in src/mixins/object_origin.mixin.js file
And some logic error in the function AdjustPosition(to).

If you can delete the changes in canvas.class.js and fix the errors in src/mixins/object_origin.mxin.js i will merge your PR. otherwise i will pull #3121 that is ready.

@asturur
Copy link
Member

asturur commented Jul 24, 2016

When you open a PR you have to check that
npm run build and npm run test run correctly.

@virror
Copy link
Contributor Author

virror commented Jul 24, 2016

@asturur Maybe its easiest for you to just pull your branch since it has all the tests and stuff as well?
For me it does not really matter : )

@asturur
Copy link
Member

asturur commented Jul 25, 2016

ok i just wanted to give you credit for the contribution. i pull the other.

@asturur asturur closed this Jul 25, 2016
@virror virror deleted the numericObjectOrigin_b branch July 25, 2016 13:28
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