-
Notifications
You must be signed in to change notification settings - Fork 76
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
(desktop/fix) Fix Owner token holder representation #13712
Conversation
687c146
to
5e148c5
Compare
Jenkins BuildsClick to see older builds (37)
|
bc6f319
to
39f6812
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM beside the alleged typo fixes ;)
ui/app/AppLayouts/Communities/popups/TokenMasterActionPopup.qml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good for me, just rename hodler back :)
453b4b7
to
b612c67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More questions :)
sensor.enabled: false | ||
|
||
Component.onCompleted: { | ||
d.updateContactDetails(model.contactId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how does this work? You're fetching and overwriting the d.contactDetails
for each delegate in the listview? 🤔 That sounds odd...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, doesn't the root.tokenOwnersModel
already include it in some way? And if not, if should probably added to it, not fetched from the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to review, from the tokenOwnersModel, what could be extracted is the name and the contactId (cf. below. The other way to go about this would be to enrich the tokenOwnersModel
with all the necessary details to be displayed and then fetch the information directly from the tokenOwners, not sure if this second approach seems overkill to me, wdyt?
Hmm, how does this work? You're fetching and overwriting the d.contactDetails for each delegate in the listview?
As of now, There is only 1 Owner token for a community so, the delegate is called once.
TokenOwnersItem* = object
contactId*: string
name*: string
imageSource*: string
numberOfMessages*: int
ownerDetails*: CollectibleOwner
amount*: Uint256
remotelyDestructState*: ContractTransactionStatus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution would be to use the LeftJoinModel
on the contactId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution would be to use the LeftJoinModel on the contactId
Sure, I think that's the easy part though.
To accomplish what is asked by @caybro
I should pull the usersModel and Join it with he tokenOwnersModel on the contactId because it's better to use the Qt binding rather than fetching the contact details even if it's called only once here as of now (I suspect it might be called more than once later on reason why I used a list...). This is ideal solution for sure, it might take me a bit more time (probably 1 more day) than I had expected judging on the usage of the contact details. I'll be working on it now CC @jrainville
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have sent a new revision making use of a LeftJoinModel for extracting the ownerToken after crossing it with the members available to the CommunitySettingsView component. Please @caybro let me know what you think.
Not directly related to this ticket, trying to understand why I don't have update of the properties... After a quick look, it seems like the Members panel as well don't have updates on the members properties. This is most likely a bug. I can have a look in a different ticket. On this screenshot the Phoenix-2 is offline in the Members list but online in the bottom left corner
69f5c6e
to
8e04f60
Compare
8e04f60
to
fc9b500
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like where this is going! :)
src/app/modules/main/communities/tokens/models/token_owners_item.nim
Outdated
Show resolved
Hide resolved
fc9b500
to
01d7034
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
@kounkou please rebase to let the tests pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
fix #12065 token owner presentation in Token section by displaying the owner token holder as contact
01d7034
to
597af88
Compare
Description
Fix #12065 token owner presentation in Token section by displaying the owner token holder as contact
What does the PR do
Fetch the contactDetails for the token owner found in the tokenOwnerModel.
Affected areas
MintedTokenView page
Screenshot of functionality (including design for comparison)
Figma:
The following match the UI expectations