-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[WIP] feat: Coachmark native popover #8095
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
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock
# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock
// Have to put the portal in a div with popover="manual" so it is also in the top layer and renders on top of the coachmark | ||
// Note, this isn't great because I'm not honestly sure what would happen if the page scrolled or their is a parent which affects that placement, is | ||
// top-layer unaffected by parent stacking context? scroll parent? | ||
let internalContainer = useRef<HTMLDivElement>(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.
could remove this if we say menus aren't allowed in coachmarks, would need more examples and somewhere to put a 'X' button maybe?
## API Changes
@react-spectrum/s2/@react-spectrum/s2:UNSTABLE_CoachMark+UNSTABLE_CoachMark {
+ UNSAFE_className?: string
+ UNSAFE_style?: CSSProperties
+ aria-describedby?: string
+ aria-details?: string
+ aria-label?: string
+ aria-labelledby?: string
+ boundaryElement?: Element = document.body
+ children: ReactNode
+ className?: string | ((PopoverRenderProps & {
+ defaultClassName: string | undefined
+})) => string
+ containerPadding?: number = 12
+ crossOffset?: number = 0
+ defaultOpen?: boolean
+ isOpen?: boolean
+ maxHeight?: number
+ offset?: number = 8
+ onOpenChange?: (boolean) => void
+ placement?: Placement = 'bottom'
+ shouldFlip?: boolean = true
+ shouldUpdatePosition?: boolean = true
+ slot?: string | null
+ style?: CSSProperties | ((PopoverRenderProps & {
+ defaultStyle: CSSProperties
+})) => CSSProperties | undefined
+ styles?: StylesProp
+} /@react-spectrum/s2:UNSTABLE_CoachMarkTrigger+UNSTABLE_CoachMarkTrigger {
+ children: ReactNode
+ isOpen?: boolean
+ onOpenChange?: (boolean) => void
+} |
{...props} | ||
<div | ||
popover="manual" | ||
role="dialog" |
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.
dialog? alert dialog? something else?
} | ||
} | ||
|
||
@starting-style { |
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 should work to put this in the style macro as a condition?
let s = style({
translateY: {
default: 0,
'@starting-style': 4
}
});
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: