Skip to content

Conversation

@lbanders
Copy link

@lbanders lbanders commented Aug 1, 2017

I have made a some updates to the Pin component to make it more usable, at least for our purposes :)
Hope you will like them and like to merge to them.

@@ -0,0 +1,3 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Please, remove ide-specific file from commit.


Required options:
* Pincode (`pinCode`) - this is the value displayed and modified by the `Pin` component
* Callback method (`onPinEntered(value)`) - this is called for everytime the use touches the keyboard (except when a digit is pressed and the `pinLength` has been reached)
Copy link
Owner

Choose a reason for hiding this comment

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

Please, rename props to value and onChange.

Nazar Matviyiv and others added 2 commits September 6, 2017 08:54
Copy link
Owner

@muromec muromec left a comment

Choose a reason for hiding this comment

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

I'd like to merge it after you fix the things I mentioned in comments above.

render() {
// Extract props
const pinCode = this.props.pinCode;
const pinLength = this.props.pinLength ? this.props.pinLength : this.defaultProps.pinLength;
Copy link
Owner

Choose a reason for hiding this comment

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

Since you fixed defaultProps defintion, you can simply do like this:

const {pinCode, pinLength, prompt} = this.props;

const dots = makeDots(pinLength - pinCode.length);

// Build buttons
const clearButton = this.buildButton(clearVisible, "Clear", () => this.doClear());
Copy link
Owner

Choose a reason for hiding this comment

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

Please, explicitly bind event handlers to this in constructor (not in render function). Something like this would do:

constructor(props) { super(props); this.handleClear = this.handleClear.bind(this); }

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants