feat: Manage Collectibles panel UI component & model #12671
Conversation
Jenkins BuildsClick to see older builds (82)
|
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
4 times, most recently
from
f79ce6b
to
d203087
Compare
| @@ -21,6 +22,8 @@ public slots: | |||
| engine->addImportPath(path); | |||
|
|
|||
| qmlRegisterSingletonType<TextUtils>("TextUtils", 1, 0, "TextUtils", &TextUtils::qmlInstance); | |||
|
|
|||
| QStandardPaths::setTestModeEnabled(true); | |||
There was a problem hiding this comment.
We don't want to overwrite user's actual settings :)
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
2 times, most recently
from
b2716d9
to
5c62a5c
Compare
✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-12671#7 🔹 ~6 min 45 sec 🔹 5c62a5c 🔹 📦 tests/nim package |
Yup, I'll create a separate issue to handle/enable that later Edit: created as #12703 |
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
from
5c62a5c
to
8fe7ed8
Compare
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
from
8fe7ed8
to
129878f
Compare
ui/app/AppLayouts/Wallet/controls/ManageTokensGroupDelegate.qml
Outdated
Show resolved
Hide resolved
| verify(!!title1) | ||
|
|
||
| // DND one item down (~80px in height) | ||
| mouseDrag(delegate0, delegate0.width/2, delegate0.height/2, 0, 80) |
There was a problem hiding this comment.
I wonder if maybe instead of using fixed value like 80 it could be possible to use value derived from delegate height, to make the test more resistant to pure-ui changes like changing delegate's height.
There was a problem hiding this comment.
Yeah, I was also thinking about that... but it's hard, the height can be variable
There was a problem hiding this comment.
But it's fine imo... if we ever change the delegate height for whatever reason, this test will start failing so we definitely notice
There was a problem hiding this comment.
But it's fine imo... if we ever change the delegate height for whatever reason, this test will start failing so we definitely notice
Yea, we will notice but will need to fix. But if it's problematic, than ok, nothing important.
| communityImage: "https://pbs.twimg.com/profile_images/1599347398769143808/C6qG3RQv_400x400.jpg", | ||
| imageUrl: "https://i.seadn.io/gae/qPfQjj4P1w0xVQXAmQJLmQ4ZtLFAJU6oiH69Lsny82LFbipLAgXhHKrcLBx2U09SmRnzeHY0ygz-3NIb-JegE_hWrZquFeL-qUPXPdw", |
There was a problem hiding this comment.
- Referring to some external resources is a bit risky, the page may become unavailable, the content may change and copyrights may be violated. So ideally we should refer to some owned resources.
- We keep some longer resources in ModelData, maybe putting some images here as base64 would be a solution.
There was a problem hiding this comment.
Hmmm... it's definitely not a (c) violation, it would be if we included it which I didn't want (in order not to bloat the already big sources). True that it might disappear any time bug really not a big deal imo
There was a problem hiding this comment.
As you prefer. I'm just pointing out that there is already a certain convention for this type of thing.
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
2 times, most recently
from
2d380f7
to
5eb205c
Compare
There was a problem hiding this comment.
Hey, I've done a visual / testing review first and meanwhile, I leave some comments / questions.. Following with the code review!
- I found some issues with the reordering of the group community and hidding / showing individual community collectibles.
There isn't a pattern I can define but here there is 1 video where the issue happens. The origin can be, from hide to show or the other way around.
-
Second 8 to 12 --> Group Pandas is in the last position, there's a collectible from this group that we hide, then the group is moved to the top position.
-
Second 20 to 25 --> Group Bearz is on top, there's a collectible from this group that is hidden and we show it, then the group is moved to the last position.
Screen.Recording.2023-11-16.at.16.11.17.mov
- Question: Looking at design there's this requirement:
`Arranging by community groups all collectibles into their respective communities:
- Collections with most collectibles are placed first
- Collectibles retain their respective order from previous view (we can’t assume the user wants to change respective ordering)`
Following this requirement, I understand that we should always place on top of the groups list, the one that has more collectibles each time the arrenge by community is switched on. Here, I don't see it's done like this, but by using the first collectible found in the flat collectibles list order. See video for better understanding:
Screen.Recording.2023-11-16.at.16.31.11.mov
Yeah, I can reproduce, will fix and add a test, thanks! :)
So the requirement is that this applies only the first time user enters the screen (before any changes are made); after that, it's the custom order that the user sets. And yeah, it's true that currently we don't (pre)sort the community collectibles that way, can you pls file a separate issue? I'll handle it in the followup PR |
There was a problem hiding this comment.
It'd be nice to have it in storybook!
There was a problem hiding this comment.
It already is, via both ManageTokensPanel.qml and ManageCollectiblesPanel.qml, doesn't make much sense to have it standalone imo
There was a problem hiding this comment.
Up to you! May be used in some point in other context!
ui/app/AppLayouts/Wallet/controls/ManageTokensGroupDelegate.qml
Outdated
Show resolved
Hide resolved
| sourceModel: root.baseModel | ||
|
|
||
| mapping: [ | ||
| RoleRename { |
There was a problem hiding this comment.
I'm not sure about that.. Isn't the base model going to have a role called symbol yet? Is / should this uid be the key role to uniqually identify the item?
There was a problem hiding this comment.
Nope, the collectibles model doesn't have a symbol role; uid is our key here
There was a problem hiding this comment.
Ok so.. uid contains information about the symbol, so we should also keep the key and the symbol. The key(although in the major cases will be == tosymbol value) will uniqualy identify the element as said before!
There was a problem hiding this comment.
There's no symbol in collectibles
There was a problem hiding this comment.
So we take the uid as the unique key (aka symbol) :)
Here the task: #12766 |
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
from
5eb205c
to
24f1dc1
Compare
- implements a QML panel to organize collectibles - factors out the delegates to separate files to be reusable with the Assets tab - adds QML tests to assess the UI functionality (move, show/hide, save/load) - does not cover the problematic "Arrange by collection" switch (TBD later) Fixes: #12379
caybro
force-pushed
the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
from
24f1dc1
to
cf4cf90
Compare
| id: root | ||
| objectName: "manageTokensDelegate-%1".arg(index) | ||
|
|
||
| // expected roles: symbol, name, communityId, communityName, communityImage, collectionName, imageUrl |
There was a problem hiding this comment.
I'd add a TODO or just a reminder here about these 2 (they are now used but will be dropped/renamed/refactored soon):
- enabledNetworkBalance
- enabledNetworkCurrencyBalance
There was a problem hiding this comment.
Thanks for the review! Added the TODO, will be in the next PR :)
caybro
deleted the
12379-wallet-token-management-collectibles-tab-ui-panel-poc
branch
Fixes: #12379
What does the PR do
Adds a Manage Collectibles UI panel component to storybook
Affected areas
storybook
Screenshot of functionality (including design for comparison)