Conversation
🦋 Changeset detectedLatest commit: fae3436 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
kostaspt
force-pushed
the
kostas/usr-248-when-sign-up-with-unverified-email-from-oauth-trigger-psu-redirects
branch
2 times, most recently
from
349a4cd to
de7fbd7
Compare
Currently, if you try to sign up with a provider that allows unverified accounts, the flow appears to be broken because it asks for the email after the OAuth callback. With this change, it will navigate to the appropriate change when needed.
kostaspt
force-pushed
the
kostas/usr-248-when-sign-up-with-unverified-email-from-oauth-trigger-psu-redirects
branch
from
de7fbd7 to
c596cf9
Compare
There was a problem hiding this comment.
Nice work @kostaspt .
Added a comment to reduce the changes on this PR, so that FE folks can review it easier and spot any important issues.
| /** | ||
| * Full URL or path to navigate after requesting phone verification. | ||
| */ | ||
| verifyPhoneNumberUrl?: string | null; |
There was a problem hiding this comment.
❓ Are these required for this fix? Seems like this is a broader API change for our components.
There was a problem hiding this comment.
It shouldn't need these, but for sign-in, using a relative URL won't work.
I want to redirect in these two cases:
sign-up/sso-callback->sign-up/verify-email-addresssign-in/sso-callback->sign-up/verify-email-address
For the first one to work, a relative URL like ../verify-email-address is working as expected, but for the second one, the hash router will be necessary to navigate here.
Or I might be possibly missing something.
There was a problem hiding this comment.
I see, thanks for explaining. I guess the problem is the the transfer flow from sign in?
We are on the sign-in/ "base" path and we're mounting components in there. Wonder if there's an alternative where we always go through sign-up/ first or we can somehow reuse the signUpUrl. I'll leave this comment open to get feedback from an expert in our javascript repo. 🙏
There was a problem hiding this comment.
Hello everyone, is there a design document explaining the changes proposed here?
There was a problem hiding this comment.
Hey @nikosdouvlis, there is no design document for this, but there is more information about the goal of this task in the ticket: https://linear.app/clerk/issue/USR-248
Do you have any concerns about these changes?
alex-ntousias
left a comment
alex-ntousias
left a comment
There was a problem hiding this comment.
Nice work @kostaspt , thank you for addressing my comments. 🚀
anagstef
left a comment
anagstef
left a comment
There was a problem hiding this comment.
This is actually a good example of when we should add an E2E test. (cc @nikosdouvlis)
…email-from-oauth-trigger-psu-redirects
kostaspt
deleted the
kostas/usr-248-when-sign-up-with-unverified-email-from-oauth-trigger-psu-redirects
branch
|
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
6 participants
Type of change
Packages affected
@clerk/clerk-js@clerk/clerk-react@clerk/nextjs@clerk/remix@clerk/types@clerk/themes@clerk/localizations@clerk/clerk-expo@clerk/backend@clerk/clerk-sdk-node@clerk/shared@clerk/fastify@clerk/chrome-extensiongatsby-plugin-clerkbuild/tooling/choreDescription
npm testruns as expected.npm run buildruns as expected.Currently, if you try to sign up with a provider that allows unverified accounts, the flow appears to be broken because it asks for the email after the OAuth callback. With this change, it will navigate to the appropriate change when needed.
Before
Kapture.2023-08-08.at.17.47.29.mp4
After
Kapture.2023-08-08.at.18.33.04.mp4