-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add documentation page for Popover component #1793
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
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
Popover
component introduced in #1791