-
Notifications
You must be signed in to change notification settings - Fork 91
feat: activities in a browser wallet #19405
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
base: master
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (71)
|
9474a38 to
856b50e
Compare
856b50e to
cd18b9b
Compare
c9d8cb9 to
c672022
Compare
fix: comments fixed
c672022 to
b4cd6e8
Compare
caybro
left a comment
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 work overall, a bit unsure about the overall architecture, esp. regarding the stores
storybook/stubs/AppLayouts/Browser/stores/BrowserActivityStore.qml
Outdated
Show resolved
Hide resolved
| import StatusQ.Core.Theme | ||
| import StatusQ.Core.Utils as SQUtils | ||
|
|
||
| import SortFilterProxyModel |
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.
Unused?
| readonly property bool isLightTheme: Theme.palette.name === Constants.lightThemeName | ||
| property color animatedBgColor | ||
| property int txType: walletRootStore.getTransactionType(root.modelData) | ||
| property int txType: activityStore.getTransactionType(root.modelData) |
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.
| property int txType: activityStore.getTransactionType(root.modelData) | |
| readonly property int txType: activityStore.getTransactionType(root.modelData) |
| } | ||
|
|
||
| function getDappDetails(chainId, contractAddress) { | ||
| return undefined |
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.
?
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 general, currently used approach is to mock stores, it's our cut-off line between ui and nim side.
For details please refer to: https://github.com/status-im/status-desktop/blob/master/guidelines/QML_ARCHITECTURE_GUIDE.md
@caybro, could you please elaborate? |
|
@caybro, @micieslak - please take a look again, review notes fixed. |
caybro
left a comment
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.
Otherwise looks good to me
| overview: root.browserWalletStore.dappBrowserAccount | ||
| communitiesStore: null | ||
| currencyStore: SharedStores.CurrenciesStore {} | ||
| networksStore: SharedStores.NetworksStore {} |
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.
These 2 stores are global, ie you should not recreate them but pass them down all the way from AppMain
|
|
||
| readonly property string selectedAddress: browserWalletStore.dappBrowserAccount.address | ||
| readonly property bool isNonArchivalNode: false | ||
| readonly property bool showAllAccounts: false |
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.
These 2 are always false?
|
|
||
| // TODO: https://github.com/status-im/status-desktop/issues/15329 | ||
| // Get DApp data from the backend | ||
| function getDappDetails(chainId, contractAddress) { |
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.
Wondering if it would make sense to have a specific Utils file for this context?
What does the PR do
Implements activities list in a browser wallet.
Affected areas
browser_section/module.nimBrowserActivityStore.qmladded for reusingHistoryView.qmlandTransactionDelegate.qmlHistoryView.qmlandTransactionDelegate.qmlpropertywalletRootStorewas renamed toactivityStore, removed wallet-specific importsArchitecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality