-
Notifications
You must be signed in to change notification settings - Fork 113
frontend: responsive grouped account selector #3717
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
frontend: responsive grouped account selector #3717
Conversation
b5ac367 to
95d32cd
Compare
frontends/web/src/components/groupedaccountselector/groupedaccountselector.tsx
Outdated
Show resolved
Hide resolved
95d32cd to
0854890
Compare
|
I'll squash the 2nd commit, just for clarity -- there I extracted the common styles between the grouped fullscreen mobile selector and the non-grouped one into one file, and use them accordingly. |
frontends/web/src/components/groupedaccountselector/groupedaccountselector.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| return ( | ||
| <> | ||
| <h1 className="title text-center">{title}</h1> |
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.
The other component that is dropdown on desktop or full screen on mobile is
input-with-dropdown.tsx. That one uses Dropdown with mobileFullScreen prop.
But here we use Select + custom full screen, and no Dropdown component and there is some code duplication.
I hoped we could get a reusable Dropdown/Fullscreen component that we can use everywhere.
Did you look into what it would take make a generic Dropdown component that can go fullscreen?
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.
Hmm fair enough. I was focused on the component itself (groupedaccountselector), but this component is already not really reusable (only for account selector, nothing else).
We should have instead a reusable "groupeddropdown" which also accepts mobileFullscreen. Or maybe extend the current dropdown so grouping works. Then we use it here instead of the default Select.
ee3e5f5 to
b5f54bd
Compare
|
Thank you for the review @thisconnect . Addressed your comment: I extended our |
Searchable responsive grouped amount selector (on mobile). Search by account name.
b5f54bd to
3d087ae
Compare
thisconnect
left a comment
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.
thank you, tested on Android LGTM
Search by account name: