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

Path 2D module #133

Merged
merged 12 commits into from
Oct 10, 2024
Merged

Path 2D module #133

merged 12 commits into from
Oct 10, 2024

Conversation

3xau1o
Copy link

@3xau1o 3xau1o commented Jul 5, 2024

As result of #132

Path2d module added with following js bindings, see on MDN

  • addPath
  • arc
  • arcTo
  • bezierCurveTo
  • closePath
  • ellipse
  • lineTo
  • moveTo
  • quadraticCurveTo
  • rect
  • roundRect

Syntax

open Webapi.Canvas

let path1 = Path2d.make()
let path2 = Path2d.make(~d="M10 10 h 80 v 80 h -80 Z", ())

path1->Path2d.addPath(path2)

path1->Path2d.arc(~x=1.,~y =2.,~r=3.,~a1 = 4., ~a2 =5.,())

Changelog

  • added Path2d module
  • added Canvas2d ellipse roundRect bindings

Breaking

  • Updated common Path2d/Canvas2d params names to match mdn docs
- ctx->quadraticCurveTo(~cp1x=1., ~cp1y=1., ~x=1., ~y=1.)
+ ctx->quadraticCurveTo(~cpx=1., ~cpy=1., ~x=1., ~y=1.)

- ctx->arc(~x=1., ~y=1., ~r=4., ~startAngle=1., ~endAngle=3., ~anticw =true)
+ ctx->arc(~x=1., ~y=1., ~r=4., ~startAngle=1., ~endAngle=3., ~counterClockWise=true, ())
  • Added Path2d.make, Dropped Canvas2d.strokePath2D
- let path: path2d = newPath2D("M24.85,10.126c2.018-4.783,6.628-8.125,11.99-8.125c7.223")
+ let path = Path2d.make(~d="M24.85,10.126c2.018-4.783,6.628-8.125,11.99-8.125c7.223")
  • Canvas2d functions taking Path2d renamed
- ctx->strokePath2D(path)
- ctx->fillPath2D(path)
+ ctx->strokePath2d(path1)
+ ctx->fillPath2d(path2)

TODO

  • add Path2d module
  • export Path2d from src/Webapi/Webapi__Canvas.res
  • remove Canvas2d.newPath2D() in favor of Path2d.make(~d=?)
  • add missing Canvas2d methods
  • Updated canvas2s test
  • extract common methods between Path2d and Context2D (Functor with parametric t)
  • list breaking changes

@3xau1o
Copy link
Author

3xau1o commented Jul 5, 2024

@TheSpyder what do you do with the generated js files from tests? are they used besides compile check?

@TheSpyder
Copy link
Owner

@utenma sorry I haven't looked at this yet. The "tests" are indeed a compile check; it ensures changes made do not impact the compiled JS (unless that's intentional).

I hope to one day have full library coverage, if you could add a few extra test calls to methods you've added that would be great 👍

Should we delete the old binding methods you're replacing?

@TheSpyder TheSpyder linked an issue Jul 11, 2024 that may be closed by this pull request
@3xau1o 3xau1o marked this pull request as ready for review July 11, 2024 20:19
@3xau1o 3xau1o requested review from spocke and TheSpyder as code owners July 11, 2024 20:19
Copy link
Owner

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

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

Getting there! just needs a few tweaks.

tests/Webapi/Canvas/Webapi__Canvas__Canvas2d__test.res Outdated Show resolved Hide resolved
src/Webapi/Canvas/Webapi__Canvas__Path_Common.res Outdated Show resolved Hide resolved
tests/Webapi/Canvas/Webapi__Canvas__Canvas2d__test.res Outdated Show resolved Hide resolved
@3xau1o
Copy link
Author

3xau1o commented Jul 29, 2024

updated list of breaking changes

@3xau1o 3xau1o requested a review from TheSpyder July 29, 2024 22:09
Copy link
Owner

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

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

I am truly sorry for letting this sit for over 2 months. I had a long work trip and then things just kept coming up that I had to deal with first.

Just one little tweak and then I'll merge+release it.

Comment on lines -120 to -122
ctx->beginPath
ctx->strokePath2D(path)
ctx->fillPath2D(path)
Copy link
Owner

Choose a reason for hiding this comment

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

These don't have an equivalent test in your new code 🤔

Copy link
Owner

@TheSpyder TheSpyder Oct 11, 2024

Choose a reason for hiding this comment

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

oh beginPath is already tested, and the others moved. My bad.

@TheSpyder TheSpyder merged commit 2fd5697 into TheSpyder:main Oct 10, 2024
@TheSpyder
Copy link
Owner

Published as 0.10.0

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.

Path2D as Module?
2 participants