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

Product table refactor #13807

Merged
merged 160 commits into from
Oct 23, 2024
Merged

Product table refactor #13807

merged 160 commits into from
Oct 23, 2024

Conversation

corwintines
Copy link
Member

Description

  • Refactor out the FindWallets table to a reusable ProductTable that can be used in other use cases (networks coming up, dapps in the future)

@corwintines
Copy link
Member Author

What is this bar on the top of the mobile filters? We don´t have it on production

Screen Shot 2024-10-18 02 21 42 PM

This was part of the drawer component, removed it for now but just saying where it came from

@corwintines
Copy link
Member Author

The head of the table has a different label. On the PR is way simpler. Was it on purpose? Screen Shot 2024-10-18 02 16 31 PM

I could update this if needed, but I generalized it in the abstraction of this component

@konopkja
Copy link
Contributor

Screenshot 2024-10-21 at 18 43 18

the tooltips position @wackerow pls check it

@wackerow
Copy link
Member

wackerow commented Oct 21, 2024

the tooltips position @wackerow pls check it

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.

🎉🎉🎉 Nice job @corwintines! This feels very robust while testing it out.


Found a few tiny nice-to-haves that I do not see as blockers, but will just note here:

  • Add type to all "@/lib/types" imports
  • "Filters" button on mobile has weird focus outline... could consider -ms-1 ps-1 to shift start edge
  • Personas on mobile still not totally clear you can scroll to the right for more
  • Getting a quick flash/glitch when opening and closing the mobile filter menu (Brave or Safari, iPhone)
  • My patch changing the Icon attribute for SwitchFilterInput may not be the most appropriate way to pass a prop, but working for now
  • Separate PR open for the icon alignment

@wackerow wackerow added the needs design approval 🧑‍🎨 Approval from a designer is needed before merging label Oct 21, 2024
@wackerow
Copy link
Member

Approved from dev side. cc: @nloureiro and/or @konopkja for approval from design side 🙏, then can pull this in (ideally before this weeks release if no critical blockers)

@konopkja
Copy link
Contributor

konopkja commented Oct 22, 2024

lgtm! last two things i found:

  1. reset button should be centered with see wallets button (vertically)
  2. this modal window has oval corners on top, should be straihgt instead
Screenshot 2024-10-22 at 11 31 06

@nloureiro
Copy link
Contributor

a media query miss alignment

Screen Shot 2024-10-22 03 17 20 PM

@konopkja
Copy link
Contributor

Screenshot 2024-10-22 at 16 21 25

when i select device, by default it does not subselect child checkboxes resulting in no filtration

expected: both ios and android are selected

@corwintines
Copy link
Member Author

Going to bring this in with @wackerow review, and approval from @nloureiro and @konopkja in Discord after fixing these last issues.

@corwintines corwintines merged commit 97c6860 into dev Oct 23, 2024
10 of 11 checks passed
@corwintines corwintines deleted the walletTableAbstraction branch October 23, 2024 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits dependencies 📦 Changes related to project dependencies needs design approval 🧑‍🎨 Approval from a designer is needed before merging tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants