Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the onChange prop not required #102

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Make the onChange prop not required #102

merged 1 commit into from
Aug 30, 2018

Conversation

ahuth
Copy link
Contributor

@ahuth ahuth commented Aug 26, 2018

Resolves #99

The component works fine when no onChange handler is provided, because an input is present with the current value. Because of that, submitting a form that this is used in will have the correct param.

I decided to accomplish this by providing a default value of () => {}. That way, existing code can call the callback, and things will still "work", even if no callback was provided.

The component works fine when no onChange handler is provided, because it creates an input component with the current value which is submitted with forms.
@hartzis hartzis self-assigned this Aug 28, 2018
@ahuth
Copy link
Contributor Author

ahuth commented Aug 28, 2018

Hey @hartzis , this solution is a little different than what you proposed in #99. Do you think it's reasonable, or are there downsides to it that I haven't thought of?

@hartzis
Copy link
Collaborator

hartzis commented Aug 30, 2018

@ahuth u rock! This feels like you jumped straight to "step profit 🤑 ".

Honestly, this absolutely should work and glad you remembered defaultProps. it is incredibly powerful.

i think i'd like to pass the creation of the recaptcha a method instead of the props.onChange down the road so it doesn't run into a possible error because it could be referencing an old onChange, but that isn't related to this issue, just made me realize that could be an issue for someone somewhere, lol.

Thank you again for the PR!

@dozoisch dozoisch merged commit 55ca14d into dozoisch:master Aug 30, 2018
@dozoisch
Copy link
Owner

@ahuth thanks, published as 1.0.1

@hartzis thanks for reviewing :)

@ahuth ahuth deleted the on-change-strictness branch August 30, 2018 17:10
@ahuth
Copy link
Contributor Author

ahuth commented Aug 30, 2018

Thanks!

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.

None yet

3 participants