-
Notifications
You must be signed in to change notification settings - Fork 280
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
Custom Pages for <UserProfile />
and <OrganizationProfile />
components
#1822
Conversation
🦋 Changeset detectedLatest commit: 3d8b86a 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 |
4f147e1
to
19a6314
Compare
!snapshot |
Hey @anagstef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/[email protected] # @clerk/chrome-extension
npm i @clerk/[email protected] # @clerk/clerk-js
npm i @clerk/[email protected] # @clerk/clerk-expo
npm i @clerk/[email protected] # @clerk/fastify
npm i @clerk/[email protected] # gatsby-plugin-clerk
npm i [email protected] # @clerk/localizations
npm i @clerk/[email protected] # @clerk/nextjs
npm i @clerk/[email protected] # @clerk/clerk-react
npm i @clerk/[email protected] # @clerk/remix
npm i @clerk/[email protected] # @clerk/clerk-sdk-node
npm i @clerk/[email protected] # @clerk/shared
npm i @clerk/[email protected] # @clerk/themes
npm i @clerk/[email protected] # @clerk/types
npm i @clerk/[email protected] |
packages/react/src/types.ts
Outdated
export type UserProfilePageProps = { | ||
url?: string; | ||
label: string; | ||
labelIcon?: React.ReactElement; |
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.
❓ Couldn't this be ReactNode ?
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.
Looking good! Nice test cases.
<Route | ||
index={!pages.isAccountPageRoot && index === 0} | ||
path={!pages.isAccountPageRoot && index === 0 ? undefined : customPage.url} | ||
key={`custom-page-${index}`} |
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 don't expect the custom pages list to change really, but using url
in the key might be better here.
const { queryParams } = useRouter(); | ||
|
||
if (componentName !== 'UserProfile') { | ||
throw new Error('Clerk: useUserProfileContext called outside of the mounted UserProfile component.'); | ||
} | ||
|
||
const pages = useMemo(() => createCustomPages(customPages || []), [customPages]); |
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 customPages
memoized upstream from this? Otherwise, this useMemo()
call might be getting recomputed on every call to this hook.
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.
From what I can tell, there isn't anything that guarantees referential equality of customPages
, so this might run more than we intend.
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.
You are right! We cannot hook on the customPages changes, because we can't really be 100% sure when custom pages contents change. So, I may just remove the useMemo
here.
@@ -43,13 +43,21 @@ const getSectionId = (id: RouteId) => `#cl-section-${id}`; | |||
|
|||
export const NavBar = (props: NavBarProps) => { | |||
const { contentRef, routes, header } = props; | |||
const [activeId, setActiveId] = React.useState<RouteId>(routes[0]['id']); | |||
const [activeId, setActiveId] = React.useState<RouteId>(''); |
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.
❓ Why are we removing this default value?
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 was causing a flicker when accessing directly a subpath in the UserProfile. In the first render, it always highlighted the first item in the navbar and then changed to the actual active. Now, it highlights nothing for half a second and then the correct item, which looks smoother in the eye.
<> | ||
<div ref={nodeRef}></div> | ||
</> |
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.
<> | |
<div ref={nodeRef}></div> | |
</> | |
<div ref={nodeRef} /> |
if (isDevelopmentEnvironment()) { | ||
checkForDuplicateUsageOfReorderingItems(customPages); | ||
} |
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.
love the helpful error checks here 👏
}; | ||
|
||
const checkForDuplicateUsageOfReorderingItems = (customPages: CustomPage[]) => { | ||
const reorderItems = customPages.filter(cp => isAccountReorderItem(cp) || isSecurityReorderItem(cp)); |
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.
Just thinking for the future, if we ever add another internal page that can be reordered, we'll have a few places that need updating. Is there a way we can consolidate that logic?
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 have refactored this part. Thanks for pointing out.
const sanitizeCustomPageURL = (url: string): string => { | ||
if (!url) { | ||
throw new Error('URL is required for custom pages'); | ||
} | ||
if (isValidUrl(url)) { | ||
throw new Error('Absolute URLs are not supported for custom pages'); | ||
} | ||
return (url as string).charAt(0) === '/' && (url as string).length > 1 ? (url as string).substring(1) : url; | ||
}; | ||
|
||
const sanitizeCustomLinkURL = (url: string): string => { | ||
if (!url) { | ||
throw new Error('URL is required for custom links'); | ||
} | ||
if (isValidUrl(url)) { | ||
return url; | ||
} | ||
return (url as string).charAt(0) === '/' ? url : `/${url}`; | ||
}; |
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.
❓ Why are we removing the leading /
in one of these methods and prepending it in another?
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 want the Custom Link items (external links) to have a leading /
so that navigate works correctly, and on the other hand Custom Page items need a relative segment, to navigate inside the UserProfile/OrganizationProfile routing mechanism.
export function UserProfilePage({ children }: PropsWithChildren<UserProfilePageProps>) { | ||
if (isDevelopmentEnvironment()) { | ||
console.error(userProfilePageRenderedError); | ||
} | ||
return <div>{children}</div>; | ||
} | ||
|
||
export function UserProfileLink({ children }: PropsWithChildren<UserProfileLinkProps>) { | ||
if (isDevelopmentEnvironment()) { | ||
console.error(userProfileLinkRenderedError); | ||
} | ||
return <div>{children}</div>; | ||
} |
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.
Should we not export these instead of logging an error when they are used directly?
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 don't export them from our package. We do this so we can import it in the useCustomPages
hook, and check if this is the component used as a child inside the UserProfile
Page: ({ children }: PropsWithChildren<UserProfilePageProps>) => React.JSX.Element; | ||
Link: ({ children }: PropsWithChildren<UserProfileLinkProps>) => React.JSX.Element; |
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.
Does this work?
Page: ({ children }: PropsWithChildren<UserProfilePageProps>) => React.JSX.Element; | |
Link: ({ children }: PropsWithChildren<UserProfileLinkProps>) => React.JSX.Element; | |
Page: typeof UserProfilePage; | |
Link: typeof UserProfileLink; |
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.
It might also be helpful to place these types at the top of the file, instead of interleaved with the source.
if (isDevelopmentEnvironment()) { | ||
console.error(userProfilePageRenderedError); | ||
} |
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.
Your errorInDevMode
helper could be used here.
!snapshot |
Hey @anagstef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/[email protected] # @clerk/chrome-extension
npm i @clerk/[email protected] # @clerk/clerk-js
npm i @clerk/[email protected] # @clerk/clerk-expo
npm i @clerk/[email protected] # @clerk/fastify
npm i @clerk/[email protected] # gatsby-plugin-clerk
npm i [email protected] # @clerk/localizations
npm i @clerk/[email protected] # @clerk/nextjs
npm i @clerk/[email protected] # @clerk/clerk-react
npm i @clerk/[email protected] # @clerk/remix
npm i @clerk/[email protected] # @clerk/clerk-sdk-node
npm i @clerk/[email protected] # @clerk/shared
npm i @clerk/[email protected] # @clerk/themes
npm i @clerk/[email protected] # @clerk/types
npm i @clerk/[email protected] |
fadab47
to
09b5917
Compare
!snapshot |
Hey @anagstef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/[email protected] # @clerk/chrome-extension
npm i @clerk/[email protected] # @clerk/clerk-js
npm i @clerk/[email protected] # @clerk/clerk-expo
npm i @clerk/[email protected] # @clerk/fastify
npm i @clerk/[email protected] # gatsby-plugin-clerk
npm i [email protected] # @clerk/localizations
npm i @clerk/[email protected] # @clerk/nextjs
npm i @clerk/[email protected] # @clerk/clerk-react
npm i @clerk/[email protected] # @clerk/remix
npm i @clerk/[email protected] # @clerk/clerk-sdk-node
npm i @clerk/[email protected] # @clerk/shared
npm i @clerk/[email protected] # @clerk/types
npm i @clerk/[email protected] |
@@ -12,65 +14,87 @@ import { VerifiedDomainPage } from './VerifiedDomainPage'; | |||
import { VerifyDomainPage } from './VerifyDomainPage'; | |||
|
|||
export const OrganizationProfileRoutes = (props: PropsOfComponent<typeof ProfileCardContent>) => { | |||
const { pages } = useOrganizationProfileContext(); | |||
const isMembersPageRoot = pages.routes[0].id === 'members'; |
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.
Any chance this array is empty?
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.
No chance; it always appends Clerk default routes.
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 there a way to use a constant or enum instead of hard-coding members
?
path='profile' | ||
flowStart | ||
index={!isPredefinedPageRoot && index === 0} | ||
path={!isPredefinedPageRoot && index === 0 ? undefined : customPage.url} |
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.
Can we extract this condition and give it some context with a good variable name?
export type NavbarItemId = 'account' | 'security' | 'members' | 'settings'; | ||
|
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.
If we are interested in keeping this we could do
export type NavbarItemId = 'account' | 'security' | 'members' | 'settings' | (string & {});
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.
Does it make sense to keep this?
|
||
const portal = () => <>{nodes[index] ? createPortal(el.component, nodes[index] as Element) : null}</>; |
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 it a common pattern to have a react hook returning JSX ?
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 it's not that common, but we need this to be updated on each render. Do you have any suggestions on different approaches?
type UserProfileCustomPage = { | ||
label: string; | ||
url: string; | ||
mountIcon: (el: HTMLDivElement) => void; | ||
unmountIcon: (el?: HTMLDivElement) => void; | ||
mount: (el: HTMLDivElement) => void; | ||
unmount: (el?: HTMLDivElement) => void; | ||
}; |
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.
🙃
type UserProfileCustomPage = { | |
label: string; | |
url: string; | |
mountIcon: (el: HTMLDivElement) => void; | |
unmountIcon: (el?: HTMLDivElement) => void; | |
mount: (el: HTMLDivElement) => void; | |
unmount: (el?: HTMLDivElement) => void; | |
}; | |
type UserProfileCustomPage = CustomPage |
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 is a different type. CustomPage
is more generic for all three types of custom items (pages, links, reordering), which is why all of its properties except label
are optional.
3e4efab
to
198cfa1
Compare
<UserProfile />
and <OrganizationProfile />
components
!snapshot |
Hey @anagstef - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/[email protected] # @clerk/chrome-extension
npm i @clerk/[email protected] # @clerk/clerk-js
npm i @clerk/[email protected] # @clerk/clerk-expo
npm i @clerk/[email protected] # @clerk/fastify
npm i @clerk/[email protected] # gatsby-plugin-clerk
npm i [email protected] # @clerk/localizations
npm i @clerk/[email protected] # @clerk/nextjs
npm i @clerk/[email protected] # @clerk/clerk-react
npm i @clerk/[email protected] # @clerk/remix
npm i @clerk/[email protected] # @clerk/clerk-sdk-node
npm i @clerk/[email protected] # @clerk/shared
npm i @clerk/[email protected] # @clerk/types
npm i @clerk/[email protected] |
93fdf20
to
593194c
Compare
6bef329
to
2053d26
Compare
…n the custom pages in dev
…ofileRoutes to be more readable
2053d26
to
3d8b86a
Compare
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. |
Description
This PR introduces custom pages and links inside the
UserProfile
andOrganizationProfile
components.UserProfile
Custom Pages
The
UserProfile
component supports the addition of custom pages. These custom pages can be rendered inside theUserProfile
component and provide a way to incorporate app-specific settings or additional functionality.To add a custom page, use the
<UserProfile.Page>
component. It accepts the following props:label
: The name that will be displayed in the navigation sidebar for the custom page.labelIcon
: An icon displayed next to the label in the navigation sidebar.url
: The path segment that will be used to navigate to the custom page. (e.g. if theUserProfile
component is rendered at/user
, then the custom page will be accessed at/user/{url}
when using path routing)children
: The components to be rendered as content inside the custom page.Types:
Example usage:
Custom Links
In addition to custom pages, you can add external links to the
UserProfile
navigation sidebar using the<UserProfile.Link>
component. It accepts the following props:label
: The name that will be displayed in the navigation sidebar for the link.labelIcon
: An icon displayed next to the label in the navigation sidebar.url
: The absolute or relative url to navigate to.Types:
Example usage:
Advanced
Reordering Default Routes
If you want to reorder the default routes (
Account
andSecurity
) in theUserProfile
navigation sidebar, you can use the<UserProfile.Page>
component with thelabel
prop set to'account'
or'security'
. This will target the existing default page and allow you to rearrange it.Example usage:
The above will result in the following order:
Notes
/
(itsurl
will be ignored) and the Clerk pages will be rendered under the path/account
.<UserProfile.Link>
component.Using custom pages with the UserButton component
If you are using the
UserButton
component with the default props (where theUserProfile
opens as a modal), then you should also be providing these custom pages as children to the component (using the<UserButton.UserProfilePage>
and<UserButton.UserProfileLink>
components respectively).Example usage:
This repetition of the same property can be avoided when the user is using the
userProfileMode='navigation'
anduserProfileUrl='<some url>'
props on theUserButton
component and has implemented a dedicated page for theUserProfile
component.OrganizationProfile
Custom Pages
The
OrganizationProfile
component supports the addition of custom pages. These custom pages can be rendered inside theOrganizationProfile
component and provide a way to incorporate organization-specific settings or additional functionality.To add a custom page, use the
<OrganizationProfile.Page>
component. It accepts the following props:label
: The name that will be displayed in the navigation sidebar for the custom page.labelIcon
: An icon displayed next to the label in the navigation sidebar.url
: The path segment that will be used to navigate to the custom page. (e.g. if theOrganizationProfile
component is rendered at/organization
, then the custom page will be accessed at/organization/{url}
when using path routing)children
: The components to be rendered as content inside the custom page.Types:
Example usage:
Custom Links
In addition to custom pages, you can add external links to the
OrganizationProfile
navigation sidebar using the<OrganizationProfile.Link>
component. It accepts the following props:label
: The name that will be displayed in the navigation sidebar for the link.labelIcon
: An icon displayed next to the label in the navigation sidebar.url
: The absolute or relative url to navigate to.Types:
Example usage:
Advanced
Reordering Default Routes
If you want to reorder the default routes (
Members
andSettings
) in theOrganizationProfile
navigation sidebar, you can use the<OrganizationProfile.Page>
component with thelabel
prop set to'members'
or'settings'
. This will target the existing default page and allow you to rearrange it.Example usage:
The above will result in the following order:
Notes
/
(itsurl
will be ignored) and the Clerk pages will be rendered under the path/organitzation-members
and/organization-settings
.<OrganizationProfile.Link>
component.Using custom pages with the OrganizationSwitcher component
If you are using the
OrganizationSwitcher
component with the default props (where theOrganizationProfile
opens as a modal), then you should also be providing these custom pages as children to the component (using the<OrganizationSwitcher.OrganizationProfilePage>
and<OrganizationSwitcher.OrganizationProfileLink>
components respectively).Example usage:
This repetition of the same property can be avoided when the user is using the
organizationProfileMode='navigation'
andorganizationProfileUrl='<some url>'
props on theOrganizationSwitcher
component and has implemented a dedicated page for theOrganizationProfile
component.Caveats
"use client";
flag.Checklist
npm test
runs as expected.npm run build
runs as expected.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-extension
gatsby-plugin-clerk
build/tooling/chore