-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: ButtonLink onClick handler #12935
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
customEventOptions, | ||
onClick, | ||
...props |
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.
Hm.. this is a little confusing since we're accepting an onClick
, which could override the click handler per below, but we also have customEventOptions
which wouldn't be used if onClick
is provided...
@nhsz If we need to accept onClick
here, should we merge it into the handleClick()
function below? ie
const handleClick = () => {
customEventOptions && trackCustomEvent(customEventOptions)
onClick()
}
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.
@nhsz @wackerow the underlying Button
component already handles this logic. So I'd say, let's get rid of the onClick
handler from this component completely and rely on the logic set in the button component. Otherwise, we are (and we were) duplicating this logic.
customEventOptions, | ||
onClick, | ||
...props |
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.
@nhsz @wackerow the underlying Button
component already handles this logic. So I'd say, let's get rid of the onClick
handler from this component completely and rely on the logic set in the button component. Otherwise, we are (and we were) duplicating this logic.
5f91ead
to
b55dc25
Compare
This PR updates the
ButtonLink
component to handle customonClick
events and override its behavior when needed. Fixes the issue of/wallets/find-wallet/
Visit website
CTA not being tracking the event when clicked.Testing
/wallets/find-wallet/
now theVisit website
button should be triggering the customhandleWalletWebsiteClick
event handler/community/
to check that theButtonLink
component is still working as expected for other casesPreview
https://deploy-preview-12935--ethereumorg.netlify.app/wallets/find-wallet