Skip to content
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 PopoverRef type #1664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add PopoverRef type #1664

wants to merge 1 commit into from

Conversation

dklymenk
Copy link

@dklymenk dklymenk commented Sep 11, 2023

Overview

Popover supports a ref prop that doesn't seem to be documented or typed.

Here is a codesandbox that showcases the ref actually working at run time, but with type errors. This PR addresses those errors by:

  • introducing a PopoverRef type that has the signatures for open and close methods
  • adding an optional ref prop to PopoverProps

I also tweaked the definition for childred prop. Specifically, the getRef prop passed to children to use the new PopoverRef instead of the generic HTMLElement.

Since the type definition for select menu relies on PopoverProps, I added the ref to the list of omitted properties as that component doesn't actually have any ref-related functionality.

The changes can be tested by trying to pass a ref created by useRef<PopoverRef> and making sure there are no type errors related to calling open or close.

Documentation

  • Updated Typescript types and/or component PropTypes
  • Added / modified component docs
  • Added / modified Storybook stories

@netlify
Copy link

netlify bot commented Sep 11, 2023

Deploy Preview for evergreen-storybook ready!

Name Link
🔨 Latest commit ee1d0dd
🔍 Latest deploy log https://app.netlify.com/sites/evergreen-storybook/deploys/64ff8cd1e9517d0009d48d67
😎 Deploy Preview https://deploy-preview-1664--evergreen-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant