Skip to content
This repository was archived by the owner on Jul 23, 2019. It is now read-only.

Add oportunity to set penColor via prop in Android#5

Open
maximromanyuk wants to merge 3 commits into
kaykaul:masterfrom
maximromanyuk:master
Open

Add oportunity to set penColor via prop in Android#5
maximromanyuk wants to merge 3 commits into
kaykaul:masterfrom
maximromanyuk:master

Conversation

@maximromanyuk

Copy link
Copy Markdown

No description provided.

@maximromanyuk maximromanyuk mentioned this pull request May 19, 2017
@maximromanyuk

Copy link
Copy Markdown
Author

@keshavkaul why not merged?

@kaykaul

kaykaul commented Jun 1, 2017

Copy link
Copy Markdown
Owner

@CandyOgre I had a busy fortnight, didn't get a chance to work on this. Will look into the PR in coming days. Thanks.

@kaykaul kaykaul left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@CandyOgre I have reviewed the PR and added comments. Do check, thanks!

Comment thread README.md
1. `selectedTool` - Set the tool id to be selected.
2. `localSourceImagePath` - Local file path of the image.
3. `onSaveSketch(saveArgs)` - Callback when saving is complete.
2. `penColor` - Set color for pen (currently only for Android).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would change the prop name from penColor to toolColor because I'm thinking of pen as one of the many tools with their own style of sketching. For example, a pencil tool might have the effect of pencil on paper. But that's currently not in the scope. But visualizing a tool having properties like color, thickness etc. is a good start.

private static final String RN_PACKAGE = "RNSketchView";

private static final String PROPS_SELECTED_TOOL = "selectedTool";
private static final String PROPS_PEN_COLOR = "penColor";

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

change to toolColor prop name

}

public void setPenColor(int toolColor) {
currentTool = new PenSketchTool(this, toolColor);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

should check if current tool implements ToolColor interface, and calls the setToolColor on the current tool if it does.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No need to create another PenSketchTool. I have already implemented setting pen color in the PenSketchTool.

paint.setStrokeCap(Paint.Cap.ROUND);
}

public PenSketchTool(View touchView, int color) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not required.

Comment thread index.js
SketchView.propTypes = {
...View.propTypes, // include the default view properties
selectedTool: PropTypes.number,
penColor: PropTypes.number,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Change to toolColor

}

@ReactProp(name = PROPS_PEN_COLOR)
public void setPenColor(SketchViewContainer viewContainer, @NonNull Integer color) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

change method name to setToolColor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants