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

multiple personas clickable [Fixes #12645] #12753

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

bhargavkakadiya
Copy link
Contributor

@bhargavkakadiya bhargavkakadiya commented Apr 17, 2024

Description

Allows multi select for personas
image

For display of wallet tables, when multiple personas are selected, filters are applied after applying AND on all persona preset filters from useWalletPersonas.tsx

The current filtering logic for displaying results is open for review. I welcome any suggestions or requirements for adjustments to ensure the functionality meets our needs

Preview link

https://deploy-preview-12753--ethereumorg.netlify.app/en/wallets/find-wallet

Related issue

Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 3df57c5
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/663fddf293d26100085d72ad
😎 Deploy Preview https://deploy-preview-12753--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 49 (🔴 down 7 from production)
Accessibility: 92 (no change from production)
Best Practices: 89 (🔴 down 9 from production)
SEO: 95 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

Thanks @bhargavkakadiya! First glance looks like a great start, but per original issue (#12645):

Switch logic to "AND" and allow users to select multiple personas.

Looks like the logic here results in an "OR" result where it'll filter wallets that match a persona OR filters that match another persona (widening results with more checks)...

@konopkja Am I correct that we want to narrow the results as more checkboxes are added?

@konopkja
Copy link
Contributor

Thanks @bhargavkakadiya! First glance looks like a great start, but per original issue (#12645):

Switch logic to "AND" and allow users to select multiple personas.

Looks like the logic here results in an "OR" result where it'll filter wallets that match a persona OR filters that match another persona (widening results with more checks)...

@konopkja Am I correct that we want to narrow the results as more checkboxes are added?

Thanks for the contribution!!! yeas our goal is to narrow down

@bhargavkakadiya
Copy link
Contributor Author

@wackerow @konopkja
Thanks for the review and inputs, I will update the logic

@bhargavkakadiya
Copy link
Contributor Author

@wackerow I have updated to show narrowed results, can you please review it again?

@konopkja
Copy link
Contributor

ping @corwintines @wackerow

@nhsz
Copy link
Member

nhsz commented Apr 30, 2024

@bhargavkakadiya in this example, end result should be 4 wallets or less, as it was narrowed to 4 when clicking the mobile filter

Kapture 2024-04-30 at 14 21 44

@wackerow
Copy link
Member

wackerow commented May 7, 2024

So close! This is definitely nuanced logic... @bhargavkakadiya Replicated Nico's steps above and it kept narrowing down appropriately, but when I uncheck the "Finance" persona, the filters do not broaden again, and the results stay filtered to only three:

Screen.Recording.2024-05-07.at.14.17.15.mov

@corwintines corwintines self-assigned this May 7, 2024
@bhargavkakadiya
Copy link
Contributor Author

In this current version, the broadening is working upon deselecting persona.

Also, based on the example below, when mobile is checked, it checks Android and iOS, which is fine.
But, when persona Finance is unchecked, it unchecks Android and iOS as well, but mobile is still checked. I will have to work on this yet, haven't figured easy fix for that yet

Screencast.from.05-14-2024.09.46.36.AM.webm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: multiple personas clickable on list of wallets
5 participants