SketchAPI icon indicating copy to clipboard operation
SketchAPI copied to clipboard

Add convert to outlines

Open WCByrne opened this issue 6 years ago • 16 comments

Addresses #408

WCByrne avatar Mar 25 '19 16:03 WCByrne

Ah sorry, I didn't comment on this.

It feels a bit weird to me to have this method on a Layer instance. Ideally, I'd prefer to have something like Layer.fromOutlines(layer) but I'm not sure where it would go.

But you are right that it can sometimes create multiple layers (when a shape has multiple borders IIRC). So I'm still not sure

mathieudutour avatar Apr 09 '19 18:04 mathieudutour

My opinion: The current name and location (layer.layersByConvertingtoOutlines) is more clear than having it on Layer. Layer.fromOutlines(layer) actually feels a little more awkward to me. If anything, maybe something like outlineLayersFromLayer(layer) but that still reads weird.

WCByrne avatar Apr 09 '19 19:04 WCByrne

yeah, I meant more about having it as a class method (eg. more like a constructor) rather than an instance method

mathieudutour avatar Apr 09 '19 19:04 mathieudutour

Any more thoughts on naming for this?

WCByrne avatar Apr 22 '19 21:04 WCByrne

@mathieudutour Let me know if and how you want me to update this. Happy to do whatever you think makes the most sense.

WCByrne avatar Apr 30 '19 21:04 WCByrne

Yeah sorry, I need to find some time to play with a few different namings. Sorry for the delay :/

mathieudutour avatar Apr 30 '19 22:04 mathieudutour

No worries, just remembered this had been sitting here for a while. This is an easy one to drop in to the internals for now.

WCByrne avatar Apr 30 '19 22:04 WCByrne

Is there a way to convert to outlines for JS Sketch plugin right now?

Thank you.

iliakan avatar Aug 10 '19 22:08 iliakan

This PR needs to address this issue that was brought up in Slack 👇 . @christianklotz any insight on how this can be achieved in code? I'd be happy to implement and get this PR moving again.

It looks like converting text to outlines is filling in letters in unexpected ways. I believe this started with 58 according my user’s reports.

image

Response: altering the fill type from Non-Zero to Even-Odd will likely achieve the intended result there (via the cog button in the Fills section within the Inspector)

WCByrne avatar Oct 23 '19 23:10 WCByrne

This is going to be fixed in Sketch 60. Once out it'd be a good time to pick up this PR again.

christianklotz avatar Oct 24 '19 13:10 christianklotz

As for failing when canConvertToOutlines is false, do the internals return nil or itself? Should that check function be adde here as well?

WCByrne avatar Sep 09 '20 14:09 WCByrne

@WCByrne it gets never called because it's only implemented for layer subclasses that support it.

christianklotz avatar Sep 09 '20 14:09 christianklotz

got it. I feel like returning nil then, or maybe even throw an error to make it clear why it didn't work. If you are trying to convert something to outlines, getting that layer back in an array seems like it could be confusing

WCByrne avatar Sep 09 '20 14:09 WCByrne

Returning nil works for me. Throwing an error is a bit harsh, especially if the method is on Layer. An alternative could be to put it on Text and ShapePath only?

christianklotz avatar Sep 09 '20 14:09 christianklotz

If those are the only 2 that support it, that works for me. I was just kinda going with what the internals seemed to do. FWIW, I only use it for text layers so this is a bit of a none issue for my needs.

WCByrne avatar Sep 09 '20 14:09 WCByrne

👍 … feel free to make the change so only the two classes have the method and I'll review.

christianklotz avatar Sep 10 '20 10:09 christianklotz