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

Kevin/fix custom shape type documentation #467

Merged

Conversation

Projects
None yet
3 participants
@KevinGutowski
Copy link
Contributor

commented Apr 13, 2019

I fixed the documentation and attempted to fix the test for ShapePath but I’m a little confused with the eslint constraint with enforcing property shorthands. When I copy the code into the Script Panel the shapePath object isn’t created. Perhaps the tests get compiled into something that Sketch can properly execute.

I am also a little confused about this comment under ShapePath.js

const ShapeTypeMapReverse = {
  Rectangle: MSRectangleShape,
  Oval: MSOvalShape,
  Polygon: MSPolygonShape,
  Star: MSStarShape,
  Triangle: MSTriangleShape,
  Custom: MSRectangleShape, // we are just going to default to Rectangle here
}

This seems to be the root of the problem. Why isn’t Custom mapped to MSShapePathLayer? This means you can’t create lines programmatically in the same way that you can with the line tool.

This PR needs more work but hopefully its helpful in pointing out the areas of issues.

Kevin Gutowski
@KevinGutowski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

I see two ways of going about fixing this. Adding a new ShapeType called “Line” or modifying ShapeTypeMapReverse

@mathieudutour

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Hum. I can see the issue. It defaults to Rectangle for backward compatibility. I added the points property recently and if you don't specify the points for a custom shape, it's not going to end up well. But maybe when points are specified when creating a Shape, we shouldn't default to Rectangle.

Adding ShapeType.Line is tricky because Sketch will always say that it's Custom. So we would need to construct the type by ourselves in the API (we could count the points).

the tests get compiled into something that Sketch can properly execute

Indeed, the tests are compiled and run using https://github.com/skpm/skpm/tree/master/packages/test-runner which uses skpm

@KevinGutowski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Gotcha, yeah it seems like I’m missing something for how to create a line. I even tried something like this and it doesn’t behave like the line tool:

let shapePathLayer = MSShapePathLayer.alloc().init()
let myLine = sketch.fromNative(shapePathLayer)
console.log(myLine.shapeType)
// 'Custom'
myLine.name = 'myLine'
myLine.frame = new Rectangle(0,0,40,100)
myLine.style = { borders: ['#FF0000'] }
myLine.points = [{ point: { x: 0, y: 0 }},{ point: { x: 1, y: 1 }}]
myLine.parent = myArtboard

I’m not sure what type of object the line tool is making exactly 🤔 . How about for now, I'll rename this PR and just fix the documentation. I’ll also add some more details to #466 about what is going on.

@KevinGutowski KevinGutowski changed the title Kevin/fix custom shape type Kevin/fix custom shape type documentation Apr 14, 2019

@mathieudutour

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Thanks for the documentation change. Could you run git reset --hard HEAD~2 in your branch to remove the last 2 commits and then run git push --force?

@KevinGutowski KevinGutowski force-pushed the KevinGutowski:kevin/fix_custom_shapeType branch from b8490e9 to 38d4af7 Apr 16, 2019

@KevinGutowski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Done!

@mathieudutour mathieudutour merged commit 1fdc45e into BohemianCoding:develop Apr 16, 2019

1 of 2 checks passed

deploy/netlify Deploy preview failed.
Details
ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.