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

Reorder collectibles #350

Merged
merged 44 commits into from
Feb 4, 2019
Merged

Reorder collectibles #350

merged 44 commits into from
Feb 4, 2019

Conversation

estebanmino
Copy link
Contributor

@estebanmino estebanmino commented Jan 25, 2019

Description

This PR reorders the way the app handles collectibles, requesting collectible contract information from OpenSea.

Following this MetaMask/website#331 (comment)

newnew

BLOCKER: MetaMask/core#56

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #342

cc @cjeria

@brunobar79
Copy link
Contributor

@estebanmino looks great!

@cjeria OpenSea API ToS require us to mention them. We should do that to avoid getting in trouble, maybe a “powered by” + logo below the collectible description text?

@estebanmino
Copy link
Contributor Author

estebanmino commented Jan 25, 2019

@brunobar79 @cjeria I forgot about the credit requirement from OpenSea I added it (is a button, going to OpenSea when clicking). We can move it

image

I also deleted a lot of code. We were using different components to render assets in the lists, now we only use AssetElement component.

@estebanmino
Copy link
Contributor Author

UPDATE

image
Found this with assets for us to use.

@brunobar79
Copy link
Contributor

brunobar79 commented Jan 25, 2019

IMO The logo looks way too big but I’ll defer to @cjeria on that.

Regarding position, i’d put it centered between address and the two bottom with a decent amount of margin bottom.

Round borders for the kudos image would be nice.

@cjeria
Copy link

cjeria commented Jan 28, 2019

This is cool, but it requires a bit more design work. @bdresser can we add this the design repo todo? cc @estebanmino @brunobar79

@bdresser
Copy link
Contributor

yep - filed here MetaMask/website#351, added to design project & public beta milestone

@estebanmino
Copy link
Contributor Author

Made a restructuration of how collectibles are baing handled from GABA and also getting collectibles information from OpenSea MetaMask/core#57. With this just pushed some changes to be in line with your feedback @cjeria

This GIF shows how collectibles work on mainnet, starting with auto-detection of a Kudos collectible.

mainnetcollectibles

While the next one shows how collectibles on testnets are being handled. We can get at least name and symbol for these collectibles. If these contracts respect ERC721 Metadata interface, we are getting name and image as well (tested with Kudos skipping OpenSea)

testnetscollectibles

This PR is getting way big and also is blocking some other work that I could do with collectibles (as implement send collectibles finished MetaMask/website#331 or block users adding collectibles that they don't own). What do you think in merging this if functional requirements are fulfilled? Then when we get the full UX/UI we could add those changes.

Thoughts? @cjeria @brunobar79 @bitpshr @bdresser

@bdresser
Copy link
Contributor

@estebanmino i think it makes sense to merge and we can revisit design later (in #351 )

Copy link
Contributor

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

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

👍

@estebanmino estebanmino merged commit 9198586 into master Feb 4, 2019
@estebanmino estebanmino deleted the reorder-collectibles branch February 4, 2019 19:47
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* render collectible in its own view

* handle collectibles without name

* wip opensea

* WIP

* rendering collectible contract list

* flow collectible contract list then collectible contract then collectible

* collectible overview

* add AssetActionButtons

* small refactor

* some ui improvements

* remove collectible contract when deleting collectible

* snapshots

* trigger onrefresh once and ui improvement

* add collectible from collectible

* open sea doc

* add locale

* handle fetch opensea errors

* AssetElement to render assets list and avoid code duplication

* delete TokenElement in favor of AssetElement

* delete collectibleElement in favor of AssetElement

* powered by opensea

* snapshots

* center collectible images

* total_supply from opensea

* add opensea logo

* snapshots

* add and remove collectible and collectibleContract from GABA

* delete opensea utils

* add collectible description

* handle collectibles by network and account

* add collectible in one method

* do not handle remove collectible contract from ui

* add collectible contract overview navbar

* render collectible contract information on modal

* handle collectibles without information

* small ui changes

* dont show opensea if testnet

* snapshots

* handle undefined collectibles attributes

* snapshots

* bump gaba

* move opensea to collectible contract information

* snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants