-
Notifications
You must be signed in to change notification settings - Fork 5
Add documentation page for Popover component #1793
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
Conversation
7be7a2a to
f8a5536
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1793 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 68 68
Lines 1220 1220
Branches 465 465
=========================================
Hits 1220 1220 ☔ View full report in Codecov by Sentry. |
1f9d03e to
ff8094c
Compare
| const [open, setOpen] = useState(false); | ||
| const buttonRef = useRef<HTMLButtonElement | null>(null); | ||
|
|
||
| useClickAway(buttonRef, () => setOpen(false)); |
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 added this because popovers handled via the popover API are automatically hidden when clicking away, which makes the open state no longer reflect reality.
I didn't notice this first because in the case of Select we have useClickAway, useFocusAway, etc.
I will address this separately, but we'll probably need to add an onClose callback to the Popover and handle a few native events, like toggle one.
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 added this because popovers handled via the popover API are automatically hidden when clicking away, which makes the open state no longer reflect reality.
This seems like an obvious hazard that we should at least document.
I will address this separately, but we'll probably need to add an onClose callback to the Popover and handle a few native events, like toggle one.
Yes, definitely. Is there an issue tracking this?
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 an issue tracking this?
Done #1799
robertknight
left a comment
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.
The documentation generally looks good but I notice the examples don't work properly if the native popover API is not available, since there is no positioned container.
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
| const [open, setOpen] = useState(false); | ||
| const buttonRef = useRef<HTMLButtonElement | null>(null); | ||
|
|
||
| useClickAway(buttonRef, () => setOpen(false)); |
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 added this because popovers handled via the popover API are automatically hidden when clicking away, which makes the open state no longer reflect reality.
This seems like an obvious hazard that we should at least document.
I will address this separately, but we'll probably need to add an onClose callback to the Popover and handle a few native events, like toggle one.
Yes, definitely. Is there an issue tracking this?
505c800 to
2f320de
Compare
ff8094c to
2386d1b
Compare
Good catch! |
2386d1b to
25265c1
Compare
src/pattern-library/components/patterns/feedback/PopoverPage.tsx
Outdated
Show resolved
Hide resolved
25265c1 to
8b15cae
Compare
Depends on #1792
Document the new
Popovercomponent introduced in #1791