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.

Sign up for GitHub

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

Jump to bottom

feat: add search feature #25170

Merged
merged 2 commits into from
Jun 13, 2024
Merged

feat: add search feature #25170

merged 2 commits into from
Jun 13, 2024

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented Jun 10, 2024 •
edited
Loading

Description

Add filter search for popular network list

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Click on the network logo
  2. you should be able to filter on network add using search input

Screenshots/Recordings

Before

before-search.mov
before-search.mov

After

after-search.mov
after-search.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@salimtb salimtb added team-assets INVALID-PR-TEMPLATE PR's body doesn't match template needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Jun 10, 2024
@salimtb salimtb requested a review from a team as a code owner June 10, 2024 10:58
@github-actions GitHub Actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Jun 10, 2024
@salimtb salimtb marked this pull request as draft June 10, 2024 11:00
@salimtb salimtb changed the title Add search feature feat: add search feature Jun 10, 2024
@salimtb salimtb changed the base branch from develop to add-popular-network-list-modal June 10, 2024 13:47
@codecov Codecov
Copy link

codecov bot commented Jun 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.63%. Comparing base (59a805a) to head (b71e51e).
Report is 5 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #25170      +/-   ##
===========================================
+ Coverage    65.61%   65.63%   +0.01%     
===========================================
  Files         1373     1374       +1     
  Lines        54523    54538      +15     
  Branches     14282    14281       -1     
===========================================
+ Hits         35774    35792      +18     
+ Misses       18749    18746       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [fc0081f]
Page Load Metrics (135 ± 183 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6814885168
domContentLoaded8211132
load401796135381183
domInteractive8211132
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -25.32 KiB (-0.73%)
  • ui: 8.91 KiB (0.13%)
  • common: 25.41 KiB (0.40%)

darkwing
darkwing previously requested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

I'm still seeing scrolling:

Screen.Recording.2024-06-10.at.6.22.26.PM.mov

ui/components/multichain/network-list-menu/network-list-search/network-list-search.tsx Outdated Show resolved Hide resolved
ui/components/multichain/network-list-menu/network-list-search/network-list-search.tsx Outdated Show resolved Hide resolved
@darkwing
Copy link
Contributor

This also looks weird:

SCR-20240610-qdsz

@salimtb salimtb force-pushed the add-popular-network-list-modal branch 3 times, most recently from dcf526b to 2a850e0 Compare June 11, 2024 09:34
@salimtb salimtb requested a review from darkwing June 11, 2024 10:27
@metamaskbot
Copy link
Collaborator

Builds ready [3e00485]
Page Load Metrics (863 ± 755 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint773991628239
domContentLoaded974312110
load4153238631572755
domInteractive874312110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -25.32 KiB (-0.73%)
  • ui: 8.71 KiB (0.13%)
  • common: 25.41 KiB (0.40%)

@salimtb salimtb marked this pull request as ready for review June 11, 2024 12:01
@salimtb salimtb force-pushed the add-search-feature branch 2 times, most recently from 6f35f63 to 3c4cb01 Compare June 11, 2024 16:03
@metamaskbot
Copy link
Collaborator

Builds ready [3c4cb01]
Page Load Metrics (147 ± 201 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint66247903919
domContentLoaded8391263
load391970147419201
domInteractive8391263
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -25.32 KiB (-0.73%)
  • ui: 8.59 KiB (0.12%)
  • common: 25.41 KiB (0.40%)

@bergeron
Copy link
Contributor

I see 1 weird behavior. When I fully backspace my search query, the focus is lost from the search box so I cannot type again. Focus seems to jump to the current network.

Screen.Recording.2024-06-11.at.9.50.01.AM.mov

Base automatically changed from add-popular-network-list-modal to develop June 11, 2024 18:09
@bergeron
Copy link
Contributor

bergeron commented Jun 11, 2024 •
edited
Loading

It also seems undesirable that the test networks aren't included in the search? They don't hide when you type something different, and it say "no networks found" even though test networks are shown

Screenshot 2024-06-11 at 11 16 28 AM

@darkwing
Copy link
Contributor

Good point Brian! I do think we should include test networks in search

@darkwing
Copy link
Contributor

I see 1 weird behavior. When I fully backspace my search query, the focus is lost from the search box so I cannot type again. Focus seems to jump to the current network.

Testing with prod version the cursor stays in place, so I think this is an undesired bug with this PR

@salimtb salimtb force-pushed the add-search-feature branch 3 times, most recently from 592fc2d to 66bf6e3 Compare June 13, 2024 08:53
@salimtb
Copy link
Contributor Author

salimtb commented Jun 13, 2024 •
edited
Loading

It also seems undesirable that the test networks aren't included in the search? They don't hide when you type something different, and it say "no networks found" even though test networks are shown

Screenshot 2024-06-11 at 11 16 28 AM

@bergeron Fixed ✅

@salimtb
Copy link
Contributor Author

salimtb commented Jun 13, 2024

Good point Brian! I do think we should include test networks in search

Test network are now included

@salimtb salimtb dismissed darkwing’s stale review June 13, 2024 09:33

comments resolved

@metamaskbot
Copy link
Collaborator

Builds ready [b71e51e]
Page Load Metrics (42 ± 1 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint62877573
domContentLoaded811910
load38484221
domInteractive811910
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.71 KiB (0.02%)
  • common: 0 Bytes (0.00%)

@salimtb salimtb merged commit 1d92608 into develop Jun 13, 2024
74 checks passed
@salimtb salimtb deleted the add-search-feature branch June 13, 2024 16:00
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darkwing darkwing darkwing approved these changes

@bergeron bergeron bergeron approved these changes

Assignees
No one assigned
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-assets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@salimtb @metamaskbot @darkwing @bergeron