Skip to content

Rename redirect and fallback url props #943

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

Merged
merged 8 commits into from
Apr 26, 2024

Conversation

S3Prototype
Copy link
Contributor

@S3Prototype S3Prototype commented Apr 24, 2024

A customer reported an issue with props like signInFallbackRedirectUrl - thread here

Changes in this PR:

  • In SignUp-related components, we should list signInFallbackRedirectUrl and fallbackRedirectUrl rather than listing signUpFallbackRedirectUrl.
  • The same should be done in reverse for SignIn-related components.
  • And we should flesh out the descriptions of these properties where appropriate

Previews:

Copy link

github-actions bot commented Apr 24, 2024

Hey, here’s your docs preview: https://clerk.com/docs/pr/943

@S3Prototype S3Prototype marked this pull request as ready for review April 24, 2024 12:56
@S3Prototype S3Prototype requested a review from a team as a code owner April 24, 2024 12:56
Copy link
Member

@royanger royanger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on . also needs updates. The props are

<SignUpButton signInFallbackRedirectUrl="" signInForceRedirectUrl="" fallbackRedirectUrl="" forceRedirectUrl="" />

For looks like we dropped the signOutCallback and instead added redirectUrl. Please also see https://clerkinc.slack.com/archives/C04KENP2GFL/p1713966411590549?thread_ts=1713901849.337169&cid=C04KENP2GFL in relation to SignOutButton as it can be passed as an option. I'm asking for clarification if that's initial.

Currently the props are:
redirectUrl
signOutOptionss
children

The SignOutOptionss is an object that looks like:

export type SignOutOptions = {
  /**
   * Specify a specific session to sign out. Useful for
   * multi-session applications.
   */
  sessionId?: string;
  /**
   * Specify a redirect URL to navigate after sign out is complete.
   */
  redirectUrl?: string;
};

@royanger
Copy link
Member

Good catch on . also needs updates. The props are

<SignUpButton signInFallbackRedirectUrl="" signInForceRedirectUrl="" fallbackRedirectUrl="" forceRedirectUrl="" />

For looks like we dropped the signOutCallback and instead added redirectUrl. Please also see https://clerkinc.slack.com/archives/C04KENP2GFL/p1713966411590549?thread_ts=1713901849.337169&cid=C04KENP2GFL in relation to SignOutButton as it can be passed as an option. I'm asking for clarification if that's initial.

Currently the props are: redirectUrl signOutOptionss children

The SignOutOptionss is an object that looks like:

export type SignOutOptions = {
  /**
   * Specify a specific session to sign out. Useful for
   * multi-session applications.
   */
  sessionId?: string;
  /**
   * Specify a redirect URL to navigate after sign out is complete.
   */
  redirectUrl?: string;
};

I believe that this has not been clarified? At the minimum signOutCallback?should be removed from . Then either add redirectUrl in its place and ship that for now, or wait on SDK to clarify and then update based on that.

Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, some minor comments regarding the order of the props

@S3Prototype S3Prototype dismissed royanger’s stale review April 26, 2024 13:59

Addressed feedback, got other approvals

@S3Prototype S3Prototype merged commit fb2d92d into main Apr 26, 2024
1 check passed
@S3Prototype S3Prototype deleted the shaquil/docs-6171-address-roys-update-requests branch April 26, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants