-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(suite-native): Create generic picker header component #16613
feat(suite-native): Create generic picker header component #16613
Conversation
🚀 Expo preview is ready!
|
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 have one small nit, but otherwise it is nice 👍
import { HStack, SearchInput, Text, VStack } from '@suite-native/atoms'; | ||
|
||
export type PickerHeaderProps = { | ||
title: string | ReactNode; |
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.
String is imo obsolete here, as the ReactNode can be string
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.
Fixed
title: string | ReactNode; | ||
children?: ReactNode; | ||
} & ( | ||
| { |
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.
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.
You might want to use utility type
RequireAllOrNone
I was unable to make it work with RequireAllOrNone
. My goal was to define type
{
title: string | ReactNode;
children?: ReactNode;
isSearchInputVisible?: false;
} | {
title: string | ReactNode;
children?: ReactNode;
isSearchInputVisible: true;
onSearchInputChange: (value: string) => void;
searchInputPlaceholder?: string;
isSearchInputDisabled?: boolean;
maxSearchInputLength?: number;
}
where only onSearchInputChange
is required. Rest of the SearchInput
props should be optional. To my understanding this is not possible with RequireAllOrNone
or any other other utility from that library. I would be happy to learn that I am mistaken.
Also all those never
types were defined just to simplify access to SearchInput
props. I've updated that code a bit, please take a look whether you like it more.
Then you can remove
isSearchInputVisible
prop
I actually considered to remove this prop, but decided to left it there for better readability. Do you prefer to remove it and decide solely on presence of onSearchInputChange
prop?
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.
Ok, keep it as it is. I did not realize that most of the props can be undefined in both cases and only onSearchInputChange
is required while is the isSearchInputVisible
set to true
.
I'm happy with the latest version 👍.
/rebase |
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13009497318 |
2fbdc22
to
46d3337
Compare
Description
Introduce
PickerHeader
andPickerCloseButton
components.Related Issue
Resolve #16605
Screenshots: