Feature to support multiple shadows

[weaintplastic]

Hi 👋🏻 ,

I'm Roland and recently fell in love with the great work that you're doing here and the endless possibilities that come with it. While already having some PR's running (you've probably noticed), I'd like to request a feature or better discuss the approach to introduce it.

Currently it is only possible to define a single box shadow. I'd like to add a feature to support multiple ones. While sketch already provides setting multiple shadows by passing in shadow styles in an array, the ViewRenderer as well as ImageRenderer do only support resolving the style for one shadow.

https://github.com/airbnb/react-sketchapp/blob/master/src/renderers/ViewRenderer.js#L48 https://github.com/airbnb/react-sketchapp/blob/master/src/renderers/ViewRenderer.js#L104

As a proposal (happy to discuss) I'd suggest to create a style property called shadowGroup in parallel. Each of them holding styles for an individual shadow. I'd still keep the current implementation in place for backwards-compatibility. So creating multiple shadows would be an opt-in functionality.

Let me know what you think.

Roland

[ianhook]

While this probably will work great in Sketch, it will not work correctly when rendered anywhere else. Setting aside whether this should allow users access to the full power of sketch, if it does do, it should do so in a way that users unfamiliar to React Native can use the component with full knowledge of what they're getting themselves into.

[jemgold]

Thanks for the proposal Roland! My thoughts === Ian's — we're designing the API of react-sketchapp to mirror react-native (or actually react-primitives). That said, there are obviously Sketch-specific features that we want to support too - Artboards & Symbols are other examples.

I appreciate you keeping backwards compatibility — I think it's low enough risk that we can include it (with the caveat that it won't work cross-platform if you're using react-primitives), and iterate on the API later if needed.

Should we leave this open for a little longer before merging to garner more feedback?

jon.gold 628.228.1152

On 4 August 2017 at 14:19, Ian Hook notifications@github.com wrote:

While this probably will work great in Sketch, it will not work correctly when rendered anywhere else. Setting aside whether this should allow users access to the full power of sketch, if it does do, it should do so in a way that users unfamiliar to React Native can use the component with full knowledge of what they're getting themselves into.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/airbnb/react-sketchapp/issues/161#issuecomment-320355605, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkHGxUidl5QsoaAI1gi_258_V-w3u_9ks5sU4rGgaJpZM4OswrK .

[weaintplastic]

Thank's so much providing some background on your opinions. Really appreciate that. I totally agree to leave this open for a little while longer and see what others think.

Roland

[mrtnbroder]

I'd agree that this project here should add as much sketch features as possible and not care so much about the cross-platform integrations. I mean, it is a fine goal to achieve cross-platform support, but since sketch is a totally different program, it is just not possible. I'd like to get all the power of sketch within this project.

[mathieudutour]

fixed in #373

[weaintplastic]

@thierryc amazing. Looking forward to the next release! @mathieudutour