fix(clerk-react): Fix forceRedirectUrl and fallbackRedirectUrl when passed to button components #3508
Conversation
🦋 Changeset detectedLatest commit: df2f087 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
| const opts: SignInProps = { | ||
| forceRedirectUrl, | ||
| fallbackRedirectUrl, |
There was a problem hiding this comment.
The modal mode relies on the non-prefixed props, adding the type here revealed the issue.
| signInFallbackRedirectUrl: fallbackRedirectUrl, | ||
| signInForceRedirectUrl: forceRedirectUrl, |
There was a problem hiding this comment.
when calling the redirect methods, the prefixed props are necessary. Added here.
| if (child && typeof child === 'object' && 'props' in child) { | ||
| await safeExecute(child.props.onClick)(e); | ||
| } |
There was a problem hiding this comment.
Some auto-fix of a lint rule caused a type error, so I tweaked the logic a bit here as a result.
| @@ -50,15 +50,13 @@ export const Modal = withFloatingTree((props: ModalProps) => { | |||
| > | |||
| <ModalContext.Provider value={modalCtx}> | |||
| <Flex | |||
| aria-hidden | |||
There was a problem hiding this comment.
I don't think we want this, as it removes the entire modal from the accessibility tree.
| fallbackRedirectUrl: url, | ||
| signUpFallbackRedirectUrl: url, |
There was a problem hiding this comment.
@BRKalow why do we need both ?
There was a problem hiding this comment.
We don't technically. The modal requires the unprefixed but the redirect method requires the prefixed. I can adjust the implementation to not pass both.
There was a problem hiding this comment.
I believe types only require the prefixed variant, so we are good
Description
When passing
forceRedirectUrlandfallbackRedirectUrltoSignInButtonandSignUpButton, the props were getting renamed and subsequently ignored by the flow when opening in a modal. I fixed the property names and added a few test cases to cover the behavior.Note that the non-modal flow wasn't broken as it expects the prefixed props (
signInForceRedirectUrl,signInFallbackRedierctUrl).Writing the tests also uncovered a bug with our modal implementation where the entire modal DOM tree was wrapped in an element that had
aria-hidden, which I fixed.Checklist
npm testruns as expected.npm run buildruns as expected.Type of change