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: add network filter to collections overview #81

Merged
merged 68 commits into from
Oct 4, 2023

Conversation

patricio0312rev
Copy link
Contributor

[collections] Add filter option per network

Summary

  • Collections can now be filtered by chain.

How is it done?

  • In the view function from CollectionController, a new filter for the chain query param has been added.
  • Here, we just use that query param to create an array of chain_ids and filter our collections in case we have 1 or more ids in the array.

Changes

  • New NetworkWithCollectionsData data constructor has been created to help us get the collections count for each network and get the correct typing.
  • New translations have been added for the new component inside pages.php
  • New CollectionsNetworksFilter component to contain all the filters.

NOTE: If you set TESTNET_ENABLED to true in your env vars, you will see the test networks in your filters.

Screen.Recording.2023-09-12.at.16.48.12.mov

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@patricio0312rev patricio0312rev marked this pull request as ready for review September 12, 2023 22:13
@patricio0312rev
Copy link
Contributor Author

Possible issues and suggestions

  • Tried to use chain[]=X&chain[]=Y, but it didn't worked correctly. I would suggest a refactor for replace-query-url.ts file. Tried to do it, but it seems a bit more complex than I thought, would like to discuss it with the team.
  • Noticed that sometimes, not all images load, especially when clicking the filters multiple times in a short span of time. I don't think it is related to any of the changes here, but would be worth checking what's going on with it in a new ticket.
  • I think a button to "Clear filters" would be nice to have and shouldn't be too hard to do if we use the hook.

Copy link

@samharperpittam samharperpittam left a comment

Choose a reason for hiding this comment

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

Quick change on this, when navigating to /collections, by default all collections are displayed across both eth and poly, but the filter shows both eth and poly deselected.

Can we change this so the network options are selected by default.

image

@samharperpittam samharperpittam marked this pull request as draft September 13, 2023 10:15
@patricio0312rev patricio0312rev marked this pull request as ready for review September 13, 2023 14:01
Copy link
Contributor

@crnkovic crnkovic left a comment

Choose a reason for hiding this comment

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

The card specifies to disable the checkbox if there are no collections for the network, but I can still toggle it:

Screenshot 2023-09-14 at 14 51 08

Also, if you hide a collection, it will still display the total number of collections per network, including hidden ones. It should probably take into account only collections based on shown/hidden state (hidden = show only hidden count per network and vice versa). cc @samharperpittam.

Screenshot 2023-09-14 at 14 56 46

EDIT: Ah, I have noticed that the shown/hidden bug only happens if you re-toggle "showHidden". If I refresh the page, the counts are okay.

app/Data/Network/NetworkWithCollectionsData.php Outdated Show resolved Hide resolved
app/Http/Controllers/CollectionController.php Show resolved Hide resolved
app/Data/Network/NetworkWithCollectionsData.php Outdated Show resolved Hide resolved
@ItsANameToo ItsANameToo marked this pull request as draft September 14, 2023 15:06
@samharperpittam
Copy link

Also, if you hide a collection, it will still display the total number of collections per network, including hidden ones. It should probably take into account only collections based on shown/hidden state (hidden = show only hidden count per network and vice versa). cc @samharperpittam.

Yeah I agree with this

@crnkovic
Copy link
Contributor

Something is screwed up with counts when showing/hiding 😂

Screen.Recording.2023-09-15.at.14.08.57.mov

@ItsANameToo ItsANameToo added this to the 0.6.0 milestone Sep 18, 2023
@ItsANameToo ItsANameToo added this to the 0.7.0 milestone Sep 22, 2023
Copy link
Contributor

@ItsANameToo ItsANameToo left a comment

Choose a reason for hiding this comment

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

still have some issues with the filter notification:

  • if you only own NFTs for one network, it will indicate it's filtered even though it technically shows all your NFTs
    image

  • when you have all networks selected and toggle "show hidden", then deselect all networks and untoggle "show hidden" you are now in a state where you can't get rid of the nofitication dot anymore
    image

  • when the page loads the NFTs, it briefly shows the filter as active before it removes the notification dot. Mainly noticeable when the user owns a lot of NFTs or if the user has no NFTs at all

image

@ItsANameToo ItsANameToo marked this pull request as draft September 22, 2023 09:05
@patricio0312rev patricio0312rev marked this pull request as ready for review September 22, 2023 10:59
@patricio0312rev
Copy link
Contributor Author

Fixed:

  1. If we only have NFTs for one network, it won't show the notification dot:
image
  1. Bug when deselecting all filters have been fixed:
image
  1. No more dot on loading:
image

@ItsANameToo ItsANameToo marked this pull request as draft September 26, 2023 09:08
@patricio0312rev patricio0312rev marked this pull request as ready for review September 26, 2023 12:54
@ItsANameToo ItsANameToo merged commit 2079e3f into develop Oct 4, 2023
16 checks passed
@ItsANameToo ItsANameToo deleted the feat/filter-option-by-network branch October 4, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants