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(multi-select): New UI component for selecting multiple networks #5143

Merged
merged 17 commits into from
Mar 26, 2024

Conversation

jh2oman
Copy link
Contributor

@jh2oman jh2oman commented Mar 22, 2024

Description

Re-usable component for doing multi selection based on the designs for selecting multiple Networks.

TODO(next): Use the new filter in Swap & Dapp filters per designs

Figma designs

Tapping around

tapping.webm

Close / Open

closing.webm

Lots of networks

many-networks.webm

Long network names

Screenshot_20240321_135129

Test plan

Unit tests

Related issues

https://linear.app/valora/issue/ACT-1098/select-token-by-network

Network scalability

Built to handle more networks, no-code required for more networks added

Copy link

emerge-tools bot commented Mar 22, 2024

1 build had no size change

Name Version Download Change Install Change Approval
Celo (test)
org.celo.mobile.test
1.81.0 (146) 24.3 MB ⬇️ 291 B 60.9 MB - N/A

Celo (test) 1.81.0 (146)
org.celo.mobile.test

No changes to report


🛸 Powered by Emerge Tools

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.79%. Comparing base (0781f6f) to head (16c9176).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5143      +/-   ##
==========================================
+ Coverage   85.76%   85.79%   +0.03%     
==========================================
  Files         740      742       +2     
  Lines       30029    30097      +68     
  Branches     5180     5188       +8     
==========================================
+ Hits        25754    25823      +69     
+ Misses       4039     4038       -1     
  Partials      236      236              
Files Coverage Δ
src/components/BottomSheetBase.tsx 83.33% <100.00%> (+0.57%) ⬆️
.../components/multiSelect/MultiSelectBottomSheet.tsx 100.00% <100.00%> (ø)
...ents/multiSelect/NetworkMultiSelectBottomSheet.tsx 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0781f6f...16c9176. Read the comment docs.

src/components/multiSelect/MultiSelectBottomSheet.tsx Outdated Show resolved Hide resolved
src/components/multiSelect/MultiSelectBottomSheet.tsx Outdated Show resolved Hide resolved
src/components/multiSelect/MultiSelectBottomSheet.tsx Outdated Show resolved Hide resolved
src/components/multiSelect/MultiSelectBottomSheet.tsx Outdated Show resolved Hide resolved
selectedOptions.filter((selectedOption) => selectedOption !== option.id)
)
} else {
setSelectedOptions([...selectedOptions, option.id])
Copy link
Contributor

Choose a reason for hiding this comment

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

could there be a race if 2 elements are clicked really fast? chatgpt suggests something like this

const toggleOption = (option) => {
  setSelectedOptions(prevSelectedOptions => {
    const isSelected = prevSelectedOptions.includes(option);

    if (isSelected) {
      return prevSelectedOptions.filter((item) => item !== option);
    } else {
      return [...prevSelectedOptions, option];
    }
  });
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah interesting! I've never used passing a function into the setState function as an updater.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually forced a much nicer unit tests!

Copy link
Contributor

@satish-ravi satish-ravi left a comment

Choose a reason for hiding this comment

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

A once build is green

@jh2oman jh2oman enabled auto-merge March 26, 2024 17:22
@jh2oman jh2oman added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 0049174 Mar 26, 2024
16 checks passed
@jh2oman jh2oman deleted the multi-select branch March 26, 2024 18:57
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
…alora-inc#5143)

### Description

Re-usable component for doing multi selection based on the designs for
selecting multiple Networks.

TODO(next): Use the new filter in Swap & Dapp filters per designs

[Figma designs

](https://www.figma.com/file/gFuRy5HJrV34ehkY0dvWWb/Swap-guidance?type=design&node-id=2632-4282&mode=design&t=xbvdPqAXfbaUiK4f-0)

#### Tapping around


[tapping.webm](https://github.com/valora-inc/wallet/assets/8432644/774a9fc8-02bc-4a91-bf1e-8b67c222c46c)


#### Close / Open


[closing.webm](https://github.com/valora-inc/wallet/assets/8432644/809f9c4d-bbff-4416-ba28-866f4b6f9cad)


#### Lots of networks


[many-networks.webm](https://github.com/valora-inc/wallet/assets/8432644/a0bf9d5a-1b8e-4e4e-a115-1a2dc4103eb6)

#### Long network names


![Screenshot_20240321_135129](https://github.com/valora-inc/wallet/assets/8432644/86bcb35b-d97a-4a81-873b-c611ffd7c743)


### Test plan

Unit tests

### Related issues

https://linear.app/valora/issue/ACT-1098/select-token-by-network


### Network scalability

Built to handle more networks, no-code required for more networks added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants