feat(CommunityOwnership): UI toasts part #12645
Conversation
Jenkins BuildsClick to see older builds (42)
|
noeliaSD
force-pushed
the
feat/toasts-issue-12175
branch
2 times, most recently
from
7dcd209
to
7c0818c
Compare
noeliaSD
requested review from
micieslak,
endulab,
caybro,
alexandraB99 and
alexjba
|
There's an extra commit added to facilitate some pr testing. It will be removed once the review is ready! Steps to test it with the dummy data added in the pr description. |
endulab
force-pushed
the
11964-setting-signer
branch
from
af968c4
to
fd8a79b
Compare
noeliaSD
force-pushed
the
feat/toasts-issue-12175
branch
from
7c0818c
to
083cbdf
Compare
endulab
force-pushed
the
11964-setting-signer
branch
2 times, most recently
from
2c1ad50
to
88240ac
Compare
noeliaSD
force-pushed
the
feat/toasts-issue-12175
branch
from
083cbdf
to
a1d8903
Compare
There was a problem hiding this comment.
Looks nice and seems to be a step into a right direction :)
However I don't fully understand some nuances here:
- Why UI intercepts some events notified by the backend, forms messages and calls back to backend in order to display it (e.g.
displayEphemeralNotification). Tehn backend exposes notifications via a model - is it necessary to somehow persist state of that model and that's the resaon why it's held on the backend side? - Now
ToastsManagerboth emitsGlobal.displayToastWithActionMessageand also handles it internally. Is the intention to finally remove that global signal and listen to all necessary stores inside manager?
ui/app/AppLayouts/Communities/panels/PermissionsSettingsPanel.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Communities/panels/PermissionsSettingsPanel.qml
Outdated
Show resolved
Hide resolved
There is a follow up task to better analyse the current implementation. This is one of the points to work on this mentioned task. If you have other concerns, please add comments in there so we can consider them later.
YES! This is the final intention, however nowadays we have other calls using the |
noeliaSD
force-pushed
the
feat/toasts-issue-12175
branch
from
a1d8903
to
9cf3088
Compare
There was a problem hiding this comment.
A good first step. 👍
Tested and work well. No regressions found.
However, I'm now sure about the actionRequired. IMO it should use the general deep links implementation (https://github.com/status-im/status-desktop/blob/master/src/app/core/custom_urls/urls_manager.nim#L56). The implementation should not be different if I trigger the action from the OS notification or the toast notification.
One other thing I've noticed is that the ToastManager doesn't really manages toasts. Currently it's more of a publisher. Not a big deal, the code is totally reusable whatever the final approach would be and I think it's good to merge as is.
Noted in the follow up task! Thanks for point that points out! |
…and new nim backend for special toasts visualization This is a first step to globalize how toasts are treated in the qml layer: - Created `ToastsManager.qml` class to deal with all app toasts generation. - Started moving community transfer ownership related toasts to the new manager class. - Some small cleanup in `AppMain.qml` Nim backend: - Created new api method to deal with extended / action toasts. - Updated needed model / item with new needed roles. Closes of #12175
noeliaSD
force-pushed
the
feat/toasts-issue-12175
branch
from
9cf3088
to
1b4352f
Compare
Part of #12175.
NOTE: It needs some other backend works in order to fully close it (missing signal
communityTokensModuleInst.sendOwnerTokenStateChanged)What does the PR do
This is a first step to globalize how toasts are treated in the qml layer:
ToastsManager.qmlclass to deal with all app toasts generation.AppMain.qmlNim backend:
NOTE: There will be a follow up task to work on the next toasts management unification refactor: #12661
Affected areas
AppMain / Toasts
CommunityTokensStore
Screenshot of functionality
Screen.Recording.2023-11-06.at.19.57.22.mov