feat(Popover): Add onChange callback #88
Conversation
src/components/Popover/index.tsx
Outdated
| /** | ||
| * Optional callback called when the popover is opened or closed | ||
| */ | ||
| onChange?: (visible: boolean) => void; |
There was a problem hiding this comment.
Should we rename it to onClose and onOpen to be more explicit? MaterialUI Popover already has a onClose property that we could use to trigger the onClose
There was a problem hiding this comment.
I was thinking about it being more in line with the ExpandableSection where there's an onChange which is passed true or false, but I guess that only makes sense if the user can control the components' state too. Will see if I can also add a visible prop so you can then do something like:
const [visible, setVisible] = useState(false);
<Popover visible={visible} onChange={setVisible}> ... </Popover>There was a problem hiding this comment.
Wow that was a learning experience! Having a visible: boolean parameter doesn't work nicely on the first render - ie visible = true still doesn't show the popover since the ref is not set until after our first render... Doesn't seem to be a neat way to get that working, so I think best not to expose a visible prop :)
Have renamed to onClose and onOpen :)
|
I think that makes sense
Sent on the go - please excuse mistakes
On Feb 19, 2021, at 2:52 PM, Jack Stevenson <notifications@github.com> wrote:
@cogwirrel commented on this pull request.
In src/components/Popover/index.tsx:
@@ -136,6 +136,11 @@ export interface PopoverProps {
* Adds an `aria-label` to the dismiss button for accessibility.
*/
dismissAriaLabel?: string;
+
+ /**
+ * Optional callback called when the popover is opened or closed
+ */
+ onChange?: (visible: boolean) => void;
I was thinking about it being more in line with the ExpandableSection where there's an onChange which is passed true or false, but I guess that only makes sense if the user can control the components' state too. Will see if I can also add a visible prop so you can then do something like:
const [visible, setVisible] = useState(false);
<Popover visible={visible} onChange={setVisible}> ... </Popover>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
## [1.0.33](v1.0.32...v1.0.33) (2021-02-19) ### Features * **Popover:** Add onOpen and onClosed callbacks ([#88](#88)) ([16cfc0e](16cfc0e))
|
🎉 This PR is included in version 1.0.33 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Issue #, if available:
Description of changes:
Adds an
onChangecallback for users so we can know when the popover was opened and closed, eg to fetch data to display in the popover.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.