SelectPanel2: Types! #3929
SelectPanel2: Types! #3929
Conversation
siddharthkp
commented
Nov 10, 2023
•
siddharthkp
commented
- Add types for experimental SelectPanel
- Removed a bunch of ts-ignores :)
|
size-limit report 📦
|
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Draft → Ready for review! |
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
This reverts commit 0afa927.
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| SelectPanel.SecondaryButton = props => { | ||
| return <Button {...props} size="small" type="button" block /> | ||
| // TODO: is this the right way to add button props? | ||
| const SelectPanelSecondaryButton: React.FC<ButtonProps> = props => { |
There was a problem hiding this comment.
Honestly this might be fine here (and is all React.FC is doing anyways) 🤷
| const SelectPanelSecondaryButton: React.FC<ButtonProps> = props => { | |
| const SelectPanelSecondaryButton = (props: ButtonProps) => { |
There was a problem hiding this comment.
Will sync with what we decide for https://github.com/primer/react/pull/3929/files#r1407598117
|
|
||
| // @ts-ignore todo | ||
| const SelectPanelSearchInput = props => { | ||
| const SelectPanelSearchInput: React.FC<TextInputProps> = ({onChange: propsOnChange, ...props}) => { |
There was a problem hiding this comment.
For onChange, do they need access to the event itself or could we provide the value of the input instead? (Or a similar onValueChange). This might help to simplify the types if onChange (or onValueChange) is something like onChange({ value }: { value: string }): void;
There was a problem hiding this comment.
Good question, I don't think there is any benefit of event here.
onValueChange is a good idea, but because this is on the text input (and not on top level), I don't think we should remove onChange (or not support it) because that feels the most natural/intuitive to expect 🤔
onValueChange could still be a useful shortcut, keeping both wouldn't simplify the types though 😅
Will think more about it, skipping for this PR
| // sync open state | ||
| React.useEffect(() => setInternalOpen(props.open), [props.open]) | ||
| React.useEffect(() => setInternalOpen(propsOpen || false), [propsOpen]) |
There was a problem hiding this comment.
It could be worth keeping track of the previous value in state instead of in an effect, I feel like this is similar to the getDerivedStateFromProps example from the hooks FAQ: https://legacy.reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops
Curious if you agree or if there is a difference here 👀
There was a problem hiding this comment.
Good call! Updated :)
| panelId: string | ||
| onCancel: () => void | ||
| onClearSelection: undefined | (() => void) | ||
| searchQuery: string | ||
| setSearchQuery: () => void | ||
| setSearchQuery: React.Dispatch<React.SetStateAction<string>> |
There was a problem hiding this comment.
Broadly speaking, how would you two feel about borrowing the createContext pattern from radix to help out with context usage like this across PRC? https://github.com/radix-ui/primitives/blob/main/packages/react/context/src/createContext.tsx#L3
I think the tl;dr is that this pattern has the context types itself be nullable (e.g. ContextValueType | null) but they leverage a hook to enforce that the value is always defined so they don't have to deal with default values.
| children: React.ReactNode | ||
| } | ||
|
|
||
| const Panel: React.FC<SelectPanelProps> = ({ |
There was a problem hiding this comment.
Super random question, is there a convention that is used to decide between React.FC<Props> versus annotating the props directly e.g. const Component = (props: Props) => ...?
There was a problem hiding this comment.
Not yet, I don't have strong opinions on this, happy to go either way.
Do we want to avoid React.FC?
Co-authored-by: Josh Black <[email protected]>
siddharthkp
had a problem deploying
to
github-pages
GitHub Actions
Failure
