Implement about tab on collectible page #18269
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (64)
|
J-Son89
reviewed
Dec 21, 2023
| [utils.re-frame :as rf])) | ||
|
|
||
| (def link-cards | ||
| [{:title "BAYC" |
There was a problem hiding this comment.
this data seems like mock data, can we move it to the temp folder so it's easier to realise this?
There was a problem hiding this comment.
or is it possible to get real data already @vkjr ??
There was a problem hiding this comment.
The ticket is to implement the UI but if you think I can get the real data, then I will 👍
There was a problem hiding this comment.
yeah if it is not too much extra work to do so, e.g the subs etc are mostly already existing then perhaps it's better to do in this pr. but if there is a follow up issue created then np either way.
We should try to minimise mock data as we approach 1.27 though as we probably shouldn't have any mock data exposed then and hide any feature that is using it if so.
J-Son89
reviewed
Dec 21, 2023
| (defn- view-internal | ||
| [] | ||
| (let [window-width (rf/sub [:dimensions/window-width]) | ||
| item-width (- (/ window-width 2) 28)] |
There was a problem hiding this comment.
what is 28? can you put that in a constant so we know what it is?
J-Son89
reviewed
Dec 21, 2023
| [quo/text | ||
| {:size :heading-2 | ||
| :weight :semi-bold} | ||
| "Bored Ape Yacht Club"]] |
There was a problem hiding this comment.
same here, with respect to mock data
J-Son89
reviewed
Dec 21, 2023
| {:style style/info-container} | ||
| [rn/view {:style style/account} | ||
| [quo/data-item | ||
| {:description :account |
There was a problem hiding this comment.
I believe we should have access to what account this is related to? 🤔
cc @vkjr, @smohamedjavid
There was a problem hiding this comment.
This one is overview tab, I think it's better to handle it in a separate ticket
There was a problem hiding this comment.
ah gotcha, the code already existed? that's fine so 👍
There was a problem hiding this comment.
yeah, this piece was implemented before status-go changes with account detail came in
There was a problem hiding this comment.
@J-Son89 I added this part and now it's dynamic. Can you please have another look
J-Son89
reviewed
Dec 21, 2023
| {:keys [traits id]} collectible-details | ||
| chain-id (get-in id [:contract-id :chain-id])] | ||
| [:<> | ||
| [info chain-id] |
There was a problem hiding this comment.
perhaps it's better to make more specific subs e.g [:wallet/last-collectible-details-chain-id] & [:wallet/last-collectible-details-traits] and call these directly inside info and traits-section components respectively.
Cc @vkjr, @ulisesmac wdyt? 🤔
There was a problem hiding this comment.
I agree,
I can see we are subscribing to :wallet/last-collectible-details, extracting a parameter (:chain-id) in a child component we are subscribing to :wallet/network-details-by-chain-id passing the previous value we extracted.
This seems that, at least, we can create a single sub dependending on :wallet/last-collectible-details and use it inside the info component to simplify this code.
There was a problem hiding this comment.
Done. I created two new subs [:wallet/last-collectible-details-chain-id] and [:wallet/last-collectible-details-traits]
mmilad75
force-pushed
the
milad/17321-about-tab-collectible-page
branch
from
f65e7e4
to
442e8cc
Compare
J-Son89
approved these changes
Jan 8, 2024
J-Son89
reviewed
Jan 8, 2024
| :status :default | ||
| :size :default | ||
| :title (i18n/label :t/network) | ||
| :network-image (quo.resources/get-network network-name) |
There was a problem hiding this comment.
I think I put the image in the sub for network details. can you check the value of (rf/sub [:wallet/network-details-by-chain-id chain-id])
There was a problem hiding this comment.
It doesn't exist there. This is the response of the reffered sub:
{:source 870, :short-name eth, :network-name :ethereum, :chain-id 1, :related-chain-id 5, :layer 1}
48% of end-end tests have passedFailed tests (22)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (23)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
|
25% of end-end tests have passedFailed tests (35)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (12)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
|
ilmotta
approved these changes
Jan 8, 2024
| :<- [:wallet/last-collectible-details] | ||
| :<- [:wallet] | ||
| (fn [[collectible wallet]] | ||
| (let [address (:address (first (:ownership collectible))) |
There was a problem hiding this comment.
The guidelines recommend testing layer-3 subs https://github.com/status-im/status-mobile/blob/563f1c588d1cdac31629da009a1c894133038d2d/doc/new-guidelines.md#registering-layer-3-subscriptions
I see other layer-3 subs in this file are not tested either.
54% of end-end tests have passedFailed tests (18)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (26)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
|
briansztamfater
pushed a commit
that referenced
this pull request
Jan 11, 2024
* add basics * finalize component * fix lint issues * move color to props * move variants to props * change file structure * working * finalize loader * fix lint issues * fix style issues * add tabs structure * add basics * draft codes * draft * make codes work after rebase * fix lint issues * remove Permissions tab * revert codes * finalize code * fix lint issues * revert podfile.lock * create new reg-sub for traits and chain-id * add real title and description for about tab * resolve comments * fix overview tab * add tests * resolve comments --------- Co-authored-by: Jamie Caprani <jamiecaprani@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17321
Figma designs: https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=1888-470765&mode=design&t=Upgum07x1OxZT1AP-4