Extend tag attributes functionality for NFTs #3162

jkadamczyk · 2022-04-06T12:14:14Z

Fixes RNBW-2832
Fixes RNBW-3140
Fixes RNBW-2840
Fixes RNBW-3203

What changed (plus any additional context for devs)

Added conversion from floating point number that was a timestamp in seconds to a formatted date string for display_type: "date" attribute from OpenSea API.
Added shortening and lower casing urls (strings starting with https://)
Added support for Boost display type from OpenSea API (display_type: "boost_number" and display_type: "boost_percentage")
Added additional sorting by display type precedence, similar to OpenSea display type grouping
add filtering for traits without values or without trait_type

PoW (screenshots / screen recordings)

Before:
https://streamable.com/fa6lby

After:
https://streamable.com/0wy2fz

Dev checklist for QA: what to test

Preferably check, Companion-in-a-box NFT attributes that return this attribute type (you can grab jkadamczyk.eth to watch)
Check if date is properly formatted
Check if the dates are last in the list
Clicking shouldn't open a context menu for date, long-press should do nothing
check links on a POAP for example (jkadamczyk.eth has POAP from dame.news)
check boosts formatting on a wallet like 0x9bD086DadfCA695B59E7bf9F4626f58900400eBF for Best Creature NFT
Boosts should have + at the beginning and if it's percent boost it should have % at the end
check sorting, precedence is like follows undefined display_type | null display_type | number display_type -> boost_percentage | boost_number display_type -> date display_type
Check link traits, these should have one or two context menu options and allow to open the URL in the web browser showing a confirmation modal before actually opening

Final checklist

Assigned individual reviewers?
Added labels?
Added e2e tests? if not please specify why – too small for e2e test it was a bug
If you added new files, did you update the CODEOWNERS file?

linear · 2022-04-06T12:14:16Z

RNBW-2832 Expiration / registration name traits are incorrectly displayed in NFT metadata

Maybe it is possible to have a generic solution to recognize if the trait value is not just a number.

If this is not possible, we can have a semi-generic solution (eg, if the trait name is ending with “date” or "day").

Update: OpenSea gives us also a display_type that includes date. Then they are grouped together on the website. We should understand them as a UNIX timestamp and express them in GTM format. Also, we shouldn't use them for grouping (like "View all nfts with this trait") - this doesn't make much sense. I don't know why do they use also fractionals (looks like this is just super precise up to nanosec), but I guess precision up to a day is sufficient

{"display_type": "date", "max_value": null, "order": null, "trait_count": 1, "trait_type": "birthday", "value": 1644018321.75},

{"display_type": "date", "max_value": null, "order": null, "trait_count": 1, "trait_type": "updated", "value": 1644018321.751}

linear · 2022-04-06T15:50:58Z

RNBW-3140 We do not understand "Boosts" properties correctly in NFTs

I am not sure how popular it is, but according to docs: https://docs.opensea.io/reference/asset-object

display_type of trait can be date (see RNBW-2832), number, null, boost_percentage, boost_number .

We do not handle correctly those two: boost_percentage and boost_number

Example: https://opensea.io/assets/0x1301566b3cb584e550a02d09562041ddc4989b91/78

OpenSea:

Rainbow:

Obviously, they are not expressed correctly (aqua power, stamina increase):

Should be +10 and +5%

Also, I think they should be the last ones in the order.

RNBW-2840 Url is capitalized as a trait value

‘Httsp://‘ looks weird. Maybe we can even cut this begging at all?

RNBW-3203 Impossible to disable long-press action on Tag component

While implementing timestamp conversion to date I noticed it is impossible to disable long-press action on a Tag component that underneath uses react-native-ios-context-menu library. We use a pretty old version of this library, we may consider upgrading it and trying if it will work.

These are the components, can be seen in Unique Asset screen

6ca0d279-8cba-4d60-917b-d323382b7196

jkadamczyk · 2022-04-06T15:52:11Z

Hey! I converted to draft, after speaking with Tomek and Terry and listening to their remarks I decided to handle all of the fixes listed as "Fixes" once since they wanted some refactors anyway.

src/helpers/uniqueAssetTraitDisplayTypeCompareFunction.ts

src/components/unique-token/UniqueTokenAttributes.tsx

src/components/unique-token/UniqueTokenAttributeItem.tsx

src/components/Tag.js

src/helpers/transformUniqueAssetTraitsForPresentation.ts

tchayen · 2022-04-06T16:41:03Z

Great job! I answered what I think I have enough context for. Hopefully others can help with the rest of the questions.

src/components/Tag.js

osdnk · 2022-04-07T08:01:55Z

src/components/unique-token/UniqueTokenAttributeItem.tsx

+    max_value?: string | number;
+  };
+
+const UniqueTokenAttributeItem: React.FC<AttributeItemProps> = ({


mmm, what's the purpose of this wrapper? Can't we just use Tag instead in src/components/unique-token/UniqueTokenAttributes.tsx?

okay, I see, this is making this mapping. But I guess we can dot his directly in Tag no?

I think we could skip it in some way @terrysahaidak what do you think about it since right now it lost the logic it had before?

Actually whatever Terry says I think it makes sense to leave that. Tag is a UI component and uses it's own naming that is very close to the visual part and is coherent with that. I think this mapping is actually not something huge and it is okay to have it to make Tah code more independent from OpenSea API. Let me know what you think

We don't use tags in any other context. This is a component made purely for OpenSea API. If you want to do mapping anyway, it's ok, but why then not do it at the parent component level?

src/components/unique-token/UniqueTokenAttributeItem.tsx

src/components/unique-token/UniqueTokenAttributes.tsx

osdnk · 2022-04-07T08:07:02Z

src/entities/uniqueAssets.ts

+export interface UniqueAssetTrait {
+  trait_type: string;
+  value: string | number;
+  display_type: string;


...text: string?

how about title? I think this should be in trait. The doc is not exhaustive in types I guess?

It is not in OpenSea docs, but I think we could add it as an optional if it gets returned

I don't think anything else belongs here to be honest. We already have everything required, some of the trait values are renamed later mapped to something else in a tag

I added max_value since given commit history it was used byt it is not documented by opensea api docs. Since it is optional it shouldn't cause any problem being there.

src/entities/uniqueAssets.ts

osdnk · 2022-04-07T08:13:18Z

src/helpers/transformUniqueAssetTraitsForPresentation.ts

+    typeof value === 'string' &&
+    value.toLocaleLowerCase().startsWith('https://')
+  ) {
+    const newValue = value.toLowerCase().replace('https://', '');


I don't mind this. I would merge this and if someone will complain, we can remove this ¯_(ツ)_/¯ .

Also, the URL should be pressable (and open the URL). Can you include this in the PR? I guess we should url?: string to MappedTrait and then use this in a Context menu

Or just check in Tag if the originalValue is an URL and defer logic to there

Yeah, good idea with opening the URL I will make sure to include it as well

Actually I think there's more to this. The context menu says sth like "search for this trait on OpenSea" I think we should entirely change the context menu there, like to include open the URL option as well. We can make this a lot earlier I guess.

Suggested change

const newValue = value.toLowerCase().replace('https://', '');

const newValue = value.substring(8);

This is what we actually want to do no?

src/helpers/uniqueAssetTraitDisplayTypeCompareFunction.ts

… for menu visibility

…onent rendered conditionally

…ng of the tag list

…format

…r know what will be opened and if they want to proceed

This reverts commit 0c37f46.

jkadamczyk · 2022-05-04T10:17:39Z

/rebase

jkadamczyk added the needs dev review Includes code review AND testing out the branch label Apr 6, 2022

jkadamczyk requested review from tchayen and osdnk April 6, 2022 12:14

jkadamczyk requested a review from terrysahaidak as a code owner April 6, 2022 12:14

jkadamczyk requested review from brunobar79 and jinchung as code owners April 6, 2022 14:58

jkadamczyk changed the title ~~Add handling of date type attribute on NFTs~~ Extend tag attributes functionality for NFTs Apr 6, 2022

jkadamczyk requested review from markdalgleish and marco as code owners April 6, 2022 15:46

jkadamczyk marked this pull request as draft April 6, 2022 15:51

jkadamczyk commented Apr 6, 2022

View reviewed changes

src/helpers/uniqueAssetTraitDisplayTypeCompareFunction.ts Outdated Show resolved Hide resolved

jkadamczyk commented Apr 6, 2022

View reviewed changes

src/components/unique-token/UniqueTokenAttributes.tsx Outdated Show resolved Hide resolved

jkadamczyk commented Apr 6, 2022

View reviewed changes

src/components/unique-token/UniqueTokenAttributeItem.tsx Outdated Show resolved Hide resolved

jkadamczyk commented Apr 6, 2022

View reviewed changes

src/components/Tag.js Outdated Show resolved Hide resolved

jkadamczyk commented Apr 6, 2022

View reviewed changes

src/components/Tag.js Outdated Show resolved Hide resolved

jkadamczyk marked this pull request as ready for review April 6, 2022 16:27

tchayen reviewed Apr 6, 2022

View reviewed changes

src/helpers/transformUniqueAssetTraitsForPresentation.ts Outdated Show resolved Hide resolved