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

feat(suite-native): Create generic picker header component #16613

Merged

Conversation

jbazant
Copy link
Contributor

@jbazant jbazant commented Jan 27, 2025

Description

Introduce PickerHeader and PickerCloseButton components.

Related Issue

Resolve #16605

Screenshots:

Screenshot 2025-01-27 at 11 14 43

@jbazant jbazant added mobile Suite Lite issues and PRs no-project This label is used to specify that PR doesn't need to be added to a project labels Jan 27, 2025
@jbazant jbazant requested a review from vytick January 27, 2025 10:49
@jbazant jbazant requested a review from a team as a code owner January 27, 2025 10:49
Copy link

github-actions bot commented Jan 27, 2025

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 23
  • More info

Learn more about 𝝠 Expo Github Action

Copy link
Contributor

@vytick vytick left a 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;
Copy link
Contributor

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

Copy link
Contributor Author

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;
} & (
| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You might want to use utility type RequireAllOrNone from type-fest library that we use in our repo (for example here).

Then you can remove isSearchInputVisible prop, because you can compute its values inside of this component based on the fact if any of the props were passed or not.

Copy link
Contributor Author

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?

Copy link
Contributor

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 👍.

@vytick
Copy link
Contributor

vytick commented Jan 28, 2025

/rebase

Copy link

@trezor-ci trezor-ci force-pushed the 16605-Mobile-Trade-Create-generic-picker-header-component branch from 2fbdc22 to 46d3337 Compare January 28, 2025 11:32
@vytick vytick merged commit 6b45921 into develop Jan 28, 2025
16 checks passed
@vytick vytick deleted the 16605-Mobile-Trade-Create-generic-picker-header-component branch January 28, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs no-project This label is used to specify that PR doesn't need to be added to a project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile Trade: Create generic picker header component
3 participants