-
Notifications
You must be signed in to change notification settings - Fork 189
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
Alternative method signature for new Matrix()
#120
Alternative method signature for new Matrix()
#120
Conversation
@@ -11,7 +11,7 @@ define([ | |||
* @constructor | |||
* @name Matrix | |||
* | |||
* @param {number} a Horizontal/x scale | |||
* @param {number|string} a Horizontal/x scale or matrix string "matrix(0,1,0,0,1,1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure of the proper jsdoc style for doing alternative signatures.
Took this approach from http://usejsdoc.org/tags-param.html under the heading Multiple types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach is good
Alternatively, is there a way with Currently I have a line of code that looks like this:
I tried doing With this pull request I could delete some other code and just say I’m using the latest commit from github version of bonsai (I’m working on the bleeding edge rather than from the official releases) |
If not, you could try this: Matrix.apply(new Matrix, '0,1,0,0,1,1'.split(',')) |
No worky. http://bit.ly/V7qtpZ |
We had some internal discussion, and the current mood is: we’d prefer new Matrix([a,b,c,d,x,y]);
new Matrix('a,b,c,d,x,y'); over new Matrix('matrix(a,b,c,d,x,y)') I hope that’s ok with you |
The reasoning behind |
It needs to be var matrix;
Matrix.apply(matrix = new Matrix, '0,1,0,0,1,1'.split(',')); |
That’s a good point. Hm. I’d personally prefer something like |
and would |
Never would’ve known that. Any link to a javascript reference as to why that is the syntax for such a thing? |
yes, like a second constructor. We just don’t have polymorphism based on signatures in JS. Would |
:D I give workshops about that stuff The short: constructors return a new instance by default. Apply calls a function as if you regularly called it, except it explicitly sets the context (new Matrix in this case) and passes on the elements of the array as if they were actual arguments to the call (see also (yes, that's the short) |
The example is too weird. var matrix = new Matrix();
Matrix.apply(matrix, '0,1,0,0,1,1'.split(',')); does the same thing. The problem with |
To the
This is what I thought I was doing with my initial code example. Apparently not :) |
My initial thought is |
I would also prefer |
ok, cool. Seems we have a winner :) |
Sweet. I’ll update. End result of pull request will include the following two signatures and one additional method: var matrix = new Matrix([a,b,c,d,e,f]);
var matrix = new Matrix(a, b, c, d, e, f);
var matrix = Matrix.fromString('a,b,c,d,e,f')
var matrix = Matrix.fromString('matrix(a,b,c,d,e,f)') |
great. If you don’t really need |
Done. |
@@ -19,6 +19,10 @@ define([ | |||
* @param {number} ty Vertical/y translation | |||
*/ | |||
function Matrix(a, b, c, d, tx, ty) { | |||
if (a instanceof Array) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last thing: could you use tools.isArray(a)
, please? And don’t forget to include the tools module ;-)
Background: The iframe runner might create sub-iframes for sub movies. Arrays from different frames might not pass this test.
Looks good otherwise. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check.
Good to know the background on that. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that “Check” meant “did it” – maybe you didn’t push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoot! It seems I did forget to push. On the bus ride home now. Will comment again when pushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I’m off for today. Will merge in the morning (CET)
For real done this time. |
Alternative method signature for `new Matrix()`
@iamdustan Thats great! Just one question, are the docs updated as well? |
Just found out that there is no need for explanation in the overview part of the docs, so no update needed. Thx! |
I am reading attributes from an SVG string and passing it to the Bonsai runner.
With the relevant attribute for a Matrix from an svg node being
transform='matrix(0,1,0,0,1,1)'
I thought it may be useful to others to have this alternative method signature.Thoughts?