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

feat: add initial transform API #23

Merged
merged 12 commits into from
Sep 3, 2024

Conversation

joseemds
Copy link
Contributor

@joseemds joseemds commented Aug 7, 2024

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.

Copy link
Owner

@ghivert ghivert left a comment

Choose a reason for hiding this comment

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

Why list? I think we can just have transform: fn (List(Transform)) -> Style, and we're good, no? We get rid of Transform, rename TransformFunction to Transform, and add None to Transform. 🙂

Also, could you please rebase on main?

sketch/src/sketch/transform.gleam Outdated Show resolved Hide resolved
sketch/src/sketch/transform.gleam Outdated Show resolved Hide resolved
@joseemds
Copy link
Contributor Author

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?

@ghivert
Copy link
Owner

ghivert commented Aug 10, 2024

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?

@joseemds
Copy link
Contributor Author

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
Copy link
Contributor Author

Rebased on main

@ghivert
Copy link
Owner

ghivert commented Aug 21, 2024

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_!"?

@joseemds
Copy link
Contributor Author

joseemds commented Sep 2, 2024

Hey, rebased on main and added transform function.

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

Copy link
Owner

@ghivert ghivert left a comment

Choose a reason for hiding this comment

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

Just a last small bugfix, and we're good to go!

sketch/src/sketch/transform.gleam Outdated Show resolved Hide resolved
@joseemds
Copy link
Contributor Author

joseemds commented Sep 2, 2024

Done!

Copy link
Owner

@ghivert ghivert left a comment

Choose a reason for hiding this comment

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

I'm sorry coming back once, everything is fine, but tests are not correct. Could you fix them?

@ghivert ghivert merged commit 34b2416 into ghivert:main Sep 3, 2024
4 checks passed
@ghivert
Copy link
Owner

ghivert commented Sep 3, 2024

Thanks a lot!

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