-
Notifications
You must be signed in to change notification settings - Fork 130
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 NavLink onClick errors when disabled #1453
Conversation
When using `NavLink` and setting `disabled={true}`, the onClick behavior causes a runtime error because React does not remove the `onClick` prop, which has the value `false`. Instead the prop is executed as a function with `.apply()` causing the runtime error, which is not defined on a boolean. Next.js warns about this in the development console.
🦋 Changeset detectedLatest commit: ae08668 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/A9fJYF9U8b2GRy5N7f4hebAxRxEh |
Needs a unit test and changset. |
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.
Thanks for the fix!
This component is kinda deprecated, where is it being used?
We are using it in the example app for authentication integration. |
Codecov Report
@@ Coverage Diff @@
## main #1453 +/- ##
=======================================
Coverage 92.20% 92.20%
=======================================
Files 195 195
Lines 3992 3992
Branches 1260 1260
=======================================
Hits 3681 3681
Misses 292 292
Partials 19 19
|
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.
We are using it in the example app for authentication integration.
Could you switch to the SideNavigation or the TopNavigation instead?
Co-authored-by: Connor Bär <connor-baer@users.noreply.github.com>
Purpose
When using
NavLink
and settingdisabled={true}
, the onClick behavior causes a runtime error because React does not remove theonClick
prop, which has the valuefalse
. Instead the prop is executed as a function with.apply()
causing the runtime error, which is not defined on a boolean.Next.js warns about this in the development console.
Approach and changes
Apply the fix recommended by Next.js so that the
onClick
prop isundefined
instead offalse
.Definition of done