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

[popover2] feat(Popover2, Tooltip2): make component type param optional #5734

Closed
wants to merge 1 commit into from

Conversation

adidahiya
Copy link
Contributor

Changes proposed in this pull request:

It turns out #5713 also had a typings regression for the popoverRef prop on datetime2 & select components, specifically the bit with React.HTMLProps<unknown>. In trying to improve this type definition, I found myself writing React.HTMLProps<HtmlElement> in a lot of places, since that's the default assumed in the definition of Popover2Props<T = ...>. It seems like we can actually just define this as the default in the Popover2 class definition and avoid specifying the type param in most cases. I'm not exactly sure why this wasn't the case before (T was required if referencing Popover2 as a type), but this will make these type references a lot simpler in our codebase and for consumers.

@adidahiya
Copy link
Contributor Author

Ok, it looks like we can't actually do this. Providing a default value for the Popover2<T> type param makes it hard to deal with the target HTML props interface in renderTarget, leading to issues like this:

Screen Shot 2022-11-08 at 7 29 39 PM

... which might be easily solved by using <Popover2<React.HTMLProps<HTMLButtonElement> in the opening tag of that code example, but we don't want to introduce a type regression which forces all consumers to instantiate the type of their Popover2s.

Opening an alternative PR to fix the type regression: #5735

@adidahiya adidahiya closed this Nov 9, 2022
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