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

Fix field names in Transform #72

Closed
artemisSystem opened this issue Jan 10, 2021 · 4 comments · Fixed by #86
Closed

Fix field names in Transform #72

artemisSystem opened this issue Jan 10, 2021 · 4 comments · Fixed by #86
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.

Comments

@artemisSystem
Copy link
Contributor

Transform currently looks like this:

type Transform =
  { m11 :: Number
  , m12 :: Number
  , m21 :: Number
  , m22 :: Number
  , m31 :: Number
  , m32 :: Number
  }

It doesn't cause any problems currently, because the arguments are just passed in order to ctx.transform and ctx.setTransform. But, the names of the last two fields are wrong, which can lead to confusion.

In setTransform and transform (MDN docs links), they are primarily referred to as a, b, c, d, e, f, but also as m11, m12, m21, m22, dx, dy.

Also, ctx.getTransform returns a DOMMatrix, whose docs page mentions that e and f are equivalent to m41 and m42 respectively, and that m31 and m32 mean something completely different, relating to 3D transformations.

I remember talking briefly about this in Slack with Phil a while ago, but forgot to open an issue/PR for it.

I think the best solution would be to change Transform to look like this:

type Transform =
  { a :: Number
  , b :: Number
  , c :: Number
  , d :: Number
  , e :: Number
  , f :: Number
  }

Would this be reasonable to have as a breaking change for 0.14? I can make a PR for it if wanted.

@JordanMartinez JordanMartinez added purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump. labels Dec 7, 2021
@JordanMartinez
Copy link
Contributor

I'm not familiar enough with this domain to respond.

@artemisSystem
Copy link
Contributor Author

Do you know who would be? It would be nice to get this change in for 0.15, since it looks like you're suggesting/wanting to make some other breaking changes as well

@JordanMartinez
Copy link
Contributor

I read through the links you posted and looked through the FFI. I also looked through #61. I agree that it makes sense to make your suggested change. Could you submit a PR?

@artemisSystem
Copy link
Contributor Author

Thanks! Yes, i'll try to remember to make a PR later today or tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants