-
Notifications
You must be signed in to change notification settings - Fork 435
feat(clerk-js): Portal option #7241
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,18 +4,27 @@ import { withCardStateProvider, withFloatingTree } from '@/ui/elements/contexts' | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Popover } from '@/ui/elements/Popover'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useUserButtonContext, withCoreUserGuard } from '../../contexts'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { usePortalContext } from '../../contexts/PortalContext'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Flow } from '../../customizables'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { usePopover } from '../../hooks'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { UserButtonPopover } from './UserButtonPopover'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { UserButtonTrigger } from './UserButtonTrigger'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const UserButtonWithFloatingTree = withFloatingTree<{ children: ReactElement }>(({ children }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { defaultOpen } = useUserButtonContext(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { defaultOpen, portal: portalProp = true } = useUserButtonContext(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get portal from context, fallback to prop, then default to true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const portalFromContext = usePortalContext(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const portal = portalProp !== undefined ? portalProp : (portalFromContext ?? true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Detect if custom portal is used (function or from context indicates intentional custom portal) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const useFixedPosition = typeof portal === 'function' || (portal !== false && portalFromContext !== undefined); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { floating, reference, styles, toggle, isOpen, nodeId, context } = usePopover({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultOpen, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| placement: 'bottom-end', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| offset: 8, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| strategy: useFixedPosition ? 'fixed' : 'absolute', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+14
to
28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Portal precedence currently ignores PortalContext and can mis-handle context The current portal resolution has two problems:
To match the comment and expected semantics (context → prop → default), you can remove the default from the destructuring and explicitly apply precedence: - const { defaultOpen, portal: portalProp = true } = useUserButtonContext();
-
- // Get portal from context, fallback to prop, then default to true
- const portalFromContext = usePortalContext();
- const portal = portalProp !== undefined ? portalProp : (portalFromContext ?? true);
-
- // Detect if custom portal is used (function or from context indicates intentional custom portal)
- const useFixedPosition = typeof portal === 'function' || (portal !== false && portalFromContext !== undefined);
+ const { defaultOpen, portal: portalProp } = useUserButtonContext();
+
+ // Get portal from context, fallback to prop, then default to true
+ const portalFromContext = usePortalContext();
+ const portal = portalFromContext ?? (portalProp ?? true);
+
+ // Detect if custom portal is used (function or from context indicates intentional custom portal)
+ const useFixedPosition =
+ typeof portal === 'function' || (portal !== false && portalFromContext !== undefined);This way:
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const userButtonMenuId = useId(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -33,6 +42,7 @@ const UserButtonWithFloatingTree = withFloatingTree<{ children: ReactElement }>( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| nodeId={nodeId} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| context={context} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isOpen={isOpen} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| portal={portal} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {cloneElement(children, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: userButtonMenuId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import type { PortalConfig } from '@clerk/shared/types'; | ||
| import React from 'react'; | ||
|
|
||
| const PortalContext = React.createContext<{ portal?: PortalConfig } | null>(null); | ||
|
|
||
| interface PortalProviderProps { | ||
| children: React.ReactNode; | ||
| portal?: PortalConfig; | ||
| } | ||
|
|
||
| export function PortalProvider({ children, portal }: PortalProviderProps): JSX.Element { | ||
| return <PortalContext.Provider value={{ portal }}>{children}</PortalContext.Provider>; | ||
| } | ||
|
|
||
| export function usePortalContext(): PortalConfig | undefined { | ||
| const context = React.useContext(PortalContext); | ||
| return context?.portal; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| import { createContextAndHook } from '@clerk/shared/react'; | ||
| import type { MenuId } from '@clerk/shared/types'; | ||
| import type { MenuId, PortalConfig } from '@clerk/shared/types'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align positioning strategy with the actual portal resolution and fix the misleading comment The new portal integration largely makes sense, but there are two small points worth addressing:
Also applies to: 30-41, 90-103 🤖 Prompt for AI Agents |
||
| import type { Placement } from '@floating-ui/react'; | ||
| import type { PropsWithChildren } from 'react'; | ||
| import React, { cloneElement, isValidElement, useLayoutEffect, useRef } from 'react'; | ||
|
|
||
| import type { Button } from '../customizables'; | ||
| import { Col, descriptors, SimpleButton } from '../customizables'; | ||
| import { usePortalContext } from '../contexts/PortalContext'; | ||
| import type { UsePopoverReturn } from '../hooks'; | ||
| import { usePopover } from '../hooks'; | ||
| import type { PropsOfComponent } from '../styledSystem'; | ||
|
|
@@ -28,10 +29,15 @@ type MenuProps = PropsWithChildren<Record<never, never>> & { | |
|
|
||
| export const Menu = withFloatingTree((props: MenuProps) => { | ||
| const { popoverPlacement = 'bottom-end', elementId, ...rest } = props; | ||
| const portalFromContext = usePortalContext(); | ||
| const useFixedPosition = | ||
| typeof portalFromContext === 'function' || (portalFromContext !== false && portalFromContext !== undefined); | ||
|
|
||
| const popoverCtx = usePopover({ | ||
| placement: popoverPlacement, | ||
| offset: 8, | ||
| shoudFlip: true, | ||
| strategy: useFixedPosition ? 'fixed' : 'absolute', | ||
| }); | ||
|
|
||
| const value = React.useMemo(() => ({ value: { popoverCtx, elementId } }), [{ ...popoverCtx }, elementId]); | ||
|
|
@@ -82,15 +88,19 @@ const findMenuItem = (el: Element, siblingType: 'prev' | 'next', options?: { cou | |
| }; | ||
|
|
||
| type MenuListProps = PropsOfComponent<typeof Col> & { | ||
| asPortal?: boolean; | ||
| asPortal?: PortalConfig; | ||
| }; | ||
|
|
||
| export const MenuList = (props: MenuListProps) => { | ||
| const { sx, asPortal, ...rest } = props; | ||
| const { sx, asPortal: asPortalProp, ...rest } = props; | ||
| const { popoverCtx, elementId } = useMenuState(); | ||
| const { floating, styles, isOpen, context, nodeId } = popoverCtx; | ||
| const containerRef = useRef<HTMLDivElement | null>(null); | ||
|
|
||
| // Get portal from context, fallback to prop, then default to false | ||
| const portalFromContext = usePortalContext(); | ||
| const asPortal = asPortalProp !== undefined ? asPortalProp : (portalFromContext ?? false); | ||
|
|
||
| useLayoutEffect(() => { | ||
| const current = containerRef.current; | ||
| floating(current); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| import type { PortalConfig } from '@clerk/shared/types'; | ||
| import type { FloatingContext, ReferenceType } from '@floating-ui/react'; | ||
| import { FloatingFocusManager, FloatingNode, FloatingPortal } from '@floating-ui/react'; | ||
| import type { PropsWithChildren } from 'react'; | ||
| import React from 'react'; | ||
|
|
||
| import { usePortalContext } from '../contexts/PortalContext'; | ||
|
|
||
| type PopoverProps = PropsWithChildren<{ | ||
| context: FloatingContext<ReferenceType>; | ||
| nodeId?: string; | ||
|
|
@@ -14,7 +17,7 @@ type PopoverProps = PropsWithChildren<{ | |
| */ | ||
| outsideElementsInert?: boolean; | ||
| order?: Array<'reference' | 'floating' | 'content'>; | ||
| portal?: boolean; | ||
| portal?: PortalConfig; | ||
| /** | ||
| * The root element to render the portal into. | ||
| * @default document.body | ||
|
|
@@ -30,41 +33,51 @@ export const Popover = (props: PopoverProps) => { | |
| order = ['reference', 'content'], | ||
| nodeId, | ||
| isOpen, | ||
| portal = true, | ||
| portal: portalProp, | ||
| root, | ||
| children, | ||
| } = props; | ||
|
|
||
| if (portal) { | ||
| // Get portal from context, fallback to prop, then default to true | ||
| const portalFromContext = usePortalContext(); | ||
| const portal = portalProp !== undefined ? portalProp : (portalFromContext ?? true); | ||
|
|
||
| // Resolve portal root | ||
| const resolveRoot = (): HTMLElement | null => { | ||
| if (typeof portal === 'function') { | ||
| return portal(); | ||
| } | ||
| if (typeof root === 'object' && 'current' in root) { | ||
| return root.current; | ||
| } | ||
| return root || null; | ||
| }; | ||
|
|
||
| const portalRoot = portal !== false ? resolveRoot() : null; | ||
| const shouldPortal = portal !== false && portalRoot !== null; | ||
|
Comment on lines
+45
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify portal logic consistency with documented defaults. The portal resolution logic (lines 41-52) results in However, the JSDoc for Please verify:
I need to verify the review comment by examining the actual codebase. Let me gather the necessary information. 🏁 Script executed: #!/bin/bash
# Find and read the Popover.tsx file
fd -t f "Popover.tsx" packages/clerk-js/src/ui/elementsLength of output: 45 🏁 Script executed: #!/bin/bash
# Search for UserProfileProps and UserButtonProps definitions
rg -n "interface UserProfileProps|interface UserButtonProps|type UserProfileProps|type UserButtonProps" -A 10 packages/clerk-js/srcLength of output: 0 🏁 Script executed: #!/bin/bash
# Read the Popover.tsx file to see full context
cat packages/clerk-js/src/ui/elements/Popover.tsxLength of output: 2090 I found the Popover.tsx file. Now I need to search for UserProfileProps and UserButtonProps to verify the JSDoc claims in the review. 🏁 Script executed: #!/bin/bash
# Search more broadly for UserProfileProps and UserButtonProps
rg -n "UserProfileProps|UserButtonProps" packages/clerk-js/src --type=ts --type=tsx | head -20Length of output: 27 🏁 Script executed: #!/bin/bash
# Search for portal prop documentation in prop interfaces
rg -n "portal\?" packages/clerk-js/src -B 5 -A 2 --type=ts --type=tsx | grep -E "(portal|@default)" | head -30Length of output: 27 🏁 Script executed: #!/bin/bash
# Find where these props might be defined
fd -t f -e "ts" -e "tsx" packages/clerk-js/src | xargs grep -l "UserProfileProps\|UserButtonProps" | head -5Length of output: 389 Let me fix the command syntax and search again: 🏁 Script executed: #!/bin/bash
# Search for UserProfileProps and UserButtonProps without type specification
rg -n "UserProfileProps|UserButtonProps" packages/clerk-js/src | head -20Length of output: 1856 🏁 Script executed: #!/bin/bash
# Search for files containing these types
fd . packages/clerk-js/src -type f | xargs grep -l "UserProfileProps\|UserButtonProps" 2>/dev/null | head -10Length of output: 170 🏁 Script executed: #!/bin/bash
# Search for portal prop in all files
rg -n "portal\?" packages/clerk-js/src -B 3 -A 1 | head -50Length of output: 2112 I can see UserProfileProps and UserButtonProps are imported from 🏁 Script executed: #!/bin/bash
# Find @clerk/shared package
find . -type d -name "shared" | grep -E "clerk|packages" | head -10Length of output: 167 🏁 Script executed: #!/bin/bash
# Search for the actual type definitions in the repo
rg -n "export.*type.*UserProfileProps|export.*interface.*UserProfileProps" . | head -10Length of output: 292 🏁 Script executed: #!/bin/bash
# Look for @clerk/shared in package.json files
rg -n "@clerk/shared" packages/clerk-js | head -5Length of output: 512 Now let me read the UserProfileProps definition and check the portal property documentation: 🏁 Script executed: #!/bin/bash
# Read the UserProfileProps definition with context
sed -n '1540,1600p' packages/shared/src/types/clerk.tsLength of output: 1960 🏁 Script executed: #!/bin/bash
# Search for portal property documentation in the shared types
rg -n "portal" packages/shared/src/types/clerk.ts -B 2 -A 1Length of output: 3073 🏁 Script executed: #!/bin/bash
# Find UserButtonProps definition
rg -n "export.*type.*UserButtonProps|export.*interface.*UserButtonProps" packages/shared/src/types/clerk.ts -A 30 | head -60Length of output: 1249 Portal default behavior in Popover contradicts JSDoc documentation. The code behavior is inconsistent with the documented defaults. When Additionally, the Popover's Fix required: Either:
|
||
|
|
||
| if (!isOpen) { | ||
| return <FloatingNode id={nodeId} />; | ||
| } | ||
|
|
||
| const content = ( | ||
| <FloatingFocusManager | ||
| context={context} | ||
| initialFocus={initialFocus} | ||
| outsideElementsInert={outsideElementsInert} | ||
| order={order} | ||
| > | ||
| <>{children}</> | ||
| </FloatingFocusManager> | ||
| ); | ||
|
|
||
| if (shouldPortal) { | ||
| return ( | ||
| <FloatingNode id={nodeId}> | ||
| <FloatingPortal root={root}> | ||
| {isOpen && ( | ||
| <FloatingFocusManager | ||
| context={context} | ||
| initialFocus={initialFocus} | ||
| outsideElementsInert={outsideElementsInert} | ||
| order={order} | ||
| > | ||
| <>{children}</> | ||
| </FloatingFocusManager> | ||
| )} | ||
| </FloatingPortal> | ||
| <FloatingPortal root={portalRoot}>{content}</FloatingPortal> | ||
| </FloatingNode> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <FloatingNode id={nodeId}> | ||
| {isOpen && ( | ||
| <FloatingFocusManager | ||
| context={context} | ||
| initialFocus={initialFocus} | ||
| order={order} | ||
| > | ||
| <>{children}</> | ||
| </FloatingFocusManager> | ||
| )} | ||
| </FloatingNode> | ||
| ); | ||
| return <FloatingNode id={nodeId}>{content}</FloatingNode>; | ||
| }; | ||
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.
PortalContext fallback is effectively disabled for OrganizationSwitcher.
portalwith a default (portal: portalProp = true), soportalPropis neverundefined.portalasportalProp !== undefined ? portalProp : (portalFromContext ?? true), which means theportalFromContextand?? truefallbacks are dead code.PortalProvider/usePortalContextcannot influence this Popover at all; only the OrganizationSwitcher context prop can, and even that defaults totrue. The comment on line 17 also no longer matches the behavior.This breaks the intent of wiring global portal configuration through
PortalContextinto OrganizationSwitcher.You can fix both the behavior and the comment with something like:
This lets:
portaloverride the global context when provided.PortalProviderportal config apply when the component doesn’t specifyportal.truewhen neither is set.Also applies to: 28-29, 46-46
🤖 Prompt for AI Agents