-
Couldn't load subscription status.
- Fork 402
fix(clerk-js): Correctly show alternative re-verification methods #5164
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(clerk-js): Correctly show alternative re-verification methods #5164
Conversation
🦋 Changeset detectedLatest commit: 11c231e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/clerk-js/src/ui/components/UserVerification/AlternativeMethods.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/UserVerification/AlternativeMethods.tsx
Outdated
Show resolved
Hide resolved
419acdf to
8f6cfb8
Compare
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 we add some unit tests for at least the 2 scenarios you mentioned in the description ?
packages/clerk-js/src/ui/components/UserVerification/UVFactorOneCodeForm.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| const hasAlternativeStrategies = | ||
| (availableFactors && availableFactors.filter(factor => isDeeplyEqual(factor, currentFactor)).length > 0) || false; |
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.
Is isDeeplyEqual necessary ? It could be an expensive operation for every render
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 could be simplified by checking the id for each strategy, will create a utility helper to check this.
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.
Done!
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 still uses isDeeplyEqual, is this intentional ?
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.
Ops, this is a leftover 🙃
7cddc3b to
a8a8982
Compare
a8a8982 to
ae5afe2
Compare
ae5afe2 to
f8cc295
Compare
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 think added a few unit tests here would be beneficial to make sure we don't break things later on.
| }; | ||
|
|
||
| const hasAlternativeStrategies = | ||
| (availableFactors && availableFactors.filter(factor => isDeeplyEqual(factor, currentFactor)).length > 0) || false; |
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 still uses isDeeplyEqual, is this intentional ?
f8cc295 to
914543a
Compare
09dc701 to
5232ca7
Compare
5232ca7 to
febd7d5
Compare
febd7d5 to
9dced53
Compare
9254503 to
11c231e
Compare
Description
This PR fixes an issue with alternative method on the user re-verification modal that was shown incorrectly in specific cases like when there was not alternative method we still showed the button to go to the alternative methods screen, or like another case where we hid alternative methods that had the same strategy but they were different.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change