Po/nfts list frontend #382
Conversation
gui/src/components/Sidebar.tsx
Outdated
| @@ -31,6 +33,7 @@ export const TABS = [ | |||
| component: Account, | |||
| icon: RequestQuoteSharpIcon, | |||
| }, | |||
| { path: "nfts", name: "NFTs", component: Nfts, icon: ImageIcon }, | |||
There was a problem hiding this comment.
instead of a new sidebar item, I think NFTs should go under the Account page.
probably later with some better navigation inside it (when many items exist), but that's for later
keeping NFTs on a separate page when Account already handles native + ERC20 is very inconsistent
There was a problem hiding this comment.
About this, agree with the consistency but I don't agree that we should place everything under "Account" and mix erc20 and erc721 together, because it might become messy.
What about changing the "account" panel to "balances" or "tokens" and keep two separated pages ?
There was a problem hiding this comment.
I have no idea about where to put this. It makes sense to put it on the Account, but that would require some sort of navigation to change between ERC20 and ERC721, while at the same time, there's plenty of space on the sidebar for now.
There was a problem hiding this comment.
The simplest approach I was thinking of this previously was to eventually have a submenu below "Account", which could just be made with anchor links within the same page. Meaning we can either scroll down and see the NFTs, or still navigate through the sidebar. for me this is the best of both worlds
the "Accounts" tab was already called "Balances" before, and the reason for changing that was precisely because I didn't want it to be just about ERC20s, but about the account as a whole
There was a problem hiding this comment.
I understand that 👍 I think my point really is that the sidebar is pretty empty, so having the "NFTs" in there feels right and doesn't require extra complexity. But, I also don't think the whole ERC20/ERC721 separation
gui/src/components/Nfts.tsx
Outdated
| setCurrentNftDetails(undefined); | ||
| setDetailsViewOpen(false); | ||
| }} | ||
| > |
There was a problem hiding this comment.
I'm gonna let @gabrielpoca review the modal part since I'm not sure if:
- this is the approach we want to go with?
- the current logic plays well with the rest of the UI? (e.g.: what if we trigger the "command bar -> open settings" option while an NFT modal is open?
There was a problem hiding this comment.
For me, the Settings are a particular type of modal, like the Command Bar, that can be opened and closed from anywhere without messing up the app's navigation. I think this modal NFT should be a page (for instance, /nfts/:id) for now. Visually, it can be a modal on top of the NFT list, as long as the settings and command bar always show up on top, but making it a page makes it so we can link to it, refresh without losing state, and avoid future problems of having way too many places where modals can be open.
This PR implements a frontend for displaying the user's NFTs.
It implements a new panel with responsive grid, that opens a modal when an item is clicked; and implements a store that fetches data every minute, or when backend emits an event.
Screen.Recording.2023-09-18.at.16.48.18.mov