search

keshavkaul / react-native-sketch-view

A React Native component for touch based drawing supporting iOS and Android.
MIT License
99 stars 47 forks source link

Color support in Props for iOS #8

Closed salockhart closed 2 years ago

salockhart commented 7 years ago

Same work as #6.

keshavkaul commented 7 years ago

@salockhart Thanks

I want to add to our previous discussion in this PR #6 :

Instead of having toolColor and toolThickness we can write a setToolProperty(toolId, propertyObj) javascript method on SketchView component that sets the property of the respective tool in native. This would be a future proof method of setting properties of a tool and backward compatible.

I don't like the concept of making tool properties visible on SketchView component. Setting these tool properties is going to be very ambiguous. In that user will be confused as to whether we are setting properties of current tool or all the tools. And if there will be more tools, do we make their properties visible to SketchView too? It's going to be a mess.

Let me know what you think. Thanks.

salockhart commented 7 years ago

I would leave the toolColor and toolThickness as props, they're just data that don't expect a return value.

The way I imagine it is to have the SketchView contain all properties about what will happen when a touch occurs. It will have options for which tool, the color, the thickness, etc. What I would imagine changes is that color and thickness will no longer be a part of the tools.

That is what occurs at the moment in this PR, except for bring the properties out of the tools themselves, which I recommend be its own issue so that these changes can be pushed through

toblerpwn commented 7 years ago

FWIW...I have to agree with @salockhart that there is a lot of value-added work ready to go sitting in PRs, and that creates a very high risk of library/usage fragmentation if they just continue to sit.

However...I also agree with @keshavkaul that setting properties on a per-tool basis feels most correct. To abstract out what exists today would essentially to have some touchOptions-type layer of abstraction that - as @salockhart says - provides the SketchView with information about what to do when a touch occurs on screen. I don't like this approach; it feels wrongly abstracted at first blush (though i could be convinced otherwise && haven't thought about it for more than 5 minutes tbh).

tl;dr, I think we should merge the open PRs from @salockhart (and others for that matter). But I also would definitely support a refactor along the lines of what @keshavkaul says.

I need to extend this functionality for a project I'm starting up this week, so ultimately it's either a question of extending this repo as an AWESOME start (which I'd prefer!) vs. forking my own (which seem to be what others are doing).

keshavkaul commented 7 years ago

@toblerpwn Thanks for providing your inputs. I've used this repo in my work, using the basic features currently supported. I do have plans to add more features like this, but am not finding the time to actively work on this. Hopefully, this will change in the coming week.

clarksandholtz commented 6 years ago

@keshavkaul any word on getting this merged? It would be a really awesome feature