-
Notifications
You must be signed in to change notification settings - Fork 1
Button: restore "inactive" prop; typescript-ify #111
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
Conversation
It's used all over qz-react. It was removed in #24 without any explanation. Seems like an oversight.
QZ-151 Social-login buttons submit the form
Steps to reproduce:
Expected results: the button click opens the 'Apple ID' popup Actual results: the button click opens the 'Apple ID' popup and logs you in This is because a |
a <button type="submit"> doesn't need an onClick handler
src/components/Button/Button.tsx
Outdated
| * element. | ||
| */ | ||
| onClick: PropTypes.func.isRequired, | ||
| onClick?: React.UIEventHandler<HTMLButtonElement>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - why make onClick optional? It seems like a useful pointer to say that maybe another kind of element is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A <button type="submit"> doesn't need (and shouldn't have) an onClick.
I think this nit should be obvious when calling the component. But when I converted to Typescript, I erroneously set this to "required" and then had to backtrack when the callers threw type-safety errors. So I've added an inline comment to save the next developer the same embarrassment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 of course! Good points.
Relates to QZ-151 in that I tried calling
<Button>from typescript and this error appeared.It looks like "inactive" might have been removed in error.
qz-reactuses it in seven different calls.