-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix(buttons): fix onClick bug #1118
Conversation
@@ -73,7 +73,7 @@ describe('rendering', () => { | |||
describe('when using "linkTo"', () => { | |||
it('should navigate to link when clicked', async () => { | |||
const { getByLabelText, history } = render( | |||
<SecondaryButton {...props} linkTo="/foo/bar" /> | |||
<SecondaryButton {...props} onClick={null} linkTo="/foo/bar" /> |
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.
this test failed in same way as the current issues in MC-FE before making the code change 👍
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.
I'm thinking if, when disabled, we could still have handleClick
return a dull function for onClick which just does () => null
. Could that save us from some of the failing tests?
I think the failing tests were due to onClick being undefined and not being disabled. |
Hmm yeah, I didn't understand it correctly before 👍 |
Summary
If the button is NOT disabled, then we should check that
onClick
exists before calling it 🤦♂This usually affects link buttons.