-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
Chore: Better typings in @grafana/ui
needed for react 18
#64828
Conversation
export const ControlledCollapse: FunctionComponent<Props> = ({ isOpen, onToggle, ...otherProps }) => { | ||
export const ControlledCollapse = ({ isOpen, onToggle, ...otherProps }: React.PropsWithChildren<Props>) => { |
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.
Alternatively, we could just type children: React.ReactNode
inProps
. At the risk of bikeshedding this, might be worth checking with broader group for what pattern we want to prefer.
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.
agreed, think it's worth discussing in a frontend weekly. don't think it needs to hold out this specific PR since we already have both throughout the codebase (😅).
i would propose using React.PropsWithChildren
more tbh. it comes from react, so if they ever change the children type (extremely unlikely i know) it would be a no-op. and it removes any chance of an incorrect type being set (what was it again... was it React.ReactNode
, React.ReactElement
or JSX.Element
? 🤔)
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.
it's just React.ReactNode. always React.ReactNode 😅
I think.
@@ -1,5 +1,5 @@ | |||
import { css, cx } from '@emotion/css'; | |||
import React, { FunctionComponent, useState } from 'react'; |
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 knew about React.FC 🤣
@@ -1,2 +0,0 @@ | |||
export type Renderable = React.ComponentType | JSX.Element; |
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.
👋🏻
(Open the links below in a new tab to go to the correct steps)
|
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.
LGTM 👍
(Levitate is complaining about the removed types RenderFunction
and Renderable
, but I couldn't find any plugins depending on it.)
What is this feature?
Why do we need this feature?
Who is this feature for?
Which issue(s) does this PR fix?:
Fixes #
Special notes for your reviewer: