sketch icon indicating copy to clipboard operation
sketch copied to clipboard

feat: add initial transform API

Open joseemds opened this issue 1 year ago • 5 comments
trafficstars

Here is a suggestion of the transform API, it was implemented based on css-transform-1 spec but i noticed that there is a newer version css-transform-2 spec that has some additions/changes.

From the first version of the spec, only the matrix() function is not implemented.

I dispose myself to update the current implementation to css-transform-2 after review of the API status, but would like to know if it is preferable to do it in this PR or create a new one.

joseemds avatar Aug 07 '24 02:08 joseemds

Having None inlined with TransformFunction would permit an API like:

sketch.transform([
  transform.none(),
  transform.rotateX(size.px(2.))
])

IMO an invalid state should not be representable, but it is a tradeoff. Maybe accept it and add documentation can be a workaround.

An alternative suggestion for the API is to expose two functions:

transform: Transform -> Style
transforms: List(Transform) -> Style

But the problem with inlining None would still exists. WDYT?

joseemds avatar Aug 10 '24 01:08 joseemds

Hmm, I did not thought about this… What about the empty list of transform functions? Isn't it the case where we'd like to output none? So we could have transform: fn (List(Transform)) -> Style, and when the list is empty, output none?

ghivert avatar Aug 10 '24 09:08 ghivert

This can be a valid solution, but IMO transform.list([]) is not transparent/explicit but documentation can fix it, i will implement it this way and we can discuss in the future other solutions

joseemds avatar Aug 13 '24 18:08 joseemds

Rebased on main

joseemds avatar Aug 13 '24 18:08 joseemds

To start, sorry for being missing so long & thanks a lot for your changes! I'd like the subject to advance before I make any change to Sketch in future, because I'm excited to get transform with a better API! 😁

I don't understand why there's so much noise in your patch set. It looks like your repo is not up-to-date with main branch. Could you try an other rebase to see if it removes the noise? Apart from this, one quick remark: there's no more transform function in sketch.gleam. I'd be happy to have transform(String) -> Style and transform_(List(Transform)) -> Style. Maybe with a comment (because it breaks Sketch conventions) "transform will be turned into transform_ in 4.0.0, and vice-versa. Meanwhile, it let you enjoy a better typed API through transform_!"?

ghivert avatar Aug 21 '24 11:08 ghivert

Hey, rebased on main and added transform function.

Now i hope everything is okay, please let me know if i should change anything

joseemds avatar Sep 02 '24 01:09 joseemds

Done!

joseemds avatar Sep 02 '24 20:09 joseemds

Thanks a lot!

ghivert avatar Sep 03 '24 00:09 ghivert