Feat/request payment modal 16736 #16744
Conversation
Cuteivist
requested review from
micieslak,
caybro,
alexjba and
a team
as code owners
Jenkins Builds
|
| if (!valid) | ||
| return 0 | ||
|
|
||
| return parseFloat(text.replace(LocaleUtils.userInputLocale.decimalPoint, ".")) |
There was a problem hiding this comment.
Are we fine with loss of precision here? 🤔
|
Build fails on all platforms |
| id: reopenButton | ||
| anchors.centerIn: parent | ||
| text: "Reopen" | ||
| enabled: !requestPaymentModalComponent.visible |
There was a problem hiding this comment.
Probably not needed. Also, here we're using the Component and visible is undefined.
| StatusDialog { | ||
| id: root | ||
|
|
||
| required property SharedStores.RequestPaymentStore store |
There was a problem hiding this comment.
WDYT of removing the store dependency here and replace it with properties? Would be much clearer what is needed for this Modal.
I see we're not using many properties/functions from the store and could be easely done.
|
|
||
| readonly property bool isSelectedHoldingValidAsset: !!selectedHolding.item | ||
|
|
||
| readonly property var adaptor: TokenSelectorViewAdaptor { |
There was a problem hiding this comment.
We have this warning in storybook:
file:///status-desktop/ui/app/AppLayouts/Wallet/adaptors/TokenSelectorViewAdaptor.qml:285:17: Unable to assign [undefined] to QAbstractItemModel*
| store: control.requestPaymentStore | ||
|
|
||
| onAccepted: { | ||
| control.requestPaymentStore.addPaymentRequest(selectedTokenKey, amount, selectedAccountAddress, selectedNetworkChainId) |
There was a problem hiding this comment.
Missing requestPaymentStore.addPaymentRequest implementation.
There was a problem hiding this comment.
Please add qml tests for the new component (or add a task to do it before we need to add more features on top of this)
There was a problem hiding this comment.
Definitely good job in general, however I have several concerns regarding the architecture which are worth amending imo:
- The modal should be fully decoupled from
StatusChatInput.StatusChatInputshould only emit signal with request to launch the modal instead of managing it internally, because it causes thatStatusChagInputdepends on everything the modal depends on now and will depend on in the future. Those two things become tightly coupled unnecessarily. RequestPaymentStoreis probably not needed at all. Broadly speaking, the store is not intended to be a container to move stuff around UI components. It's to expose backend's functionality. And every backend's mode/signal/function should be exposed only once via a single store.- The
RequestPaymentModalshould be a plain UI component taking some models and emitting some signals with no dependency on any store. Also transformations like that done byTokenSelectorViewAdaptorshould not be done internally. Instead the modal should depend on the model. It's up to the caller to compose data using any adaptor that's needed.
In case of any doubts please ping me, I will be happy to discuss about that.
todo: remove unused imports todo: finish handling amount input
Cuteivist
force-pushed
the
feat/request-payment-modal-16736
branch
from
cfc1b4c
to
6556a35
Compare
Closes #16736
What does the PR do
Affected areas
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
Impact on end user
What is the impact of these changes on the end user (before/after behaviour)
How to test
Enable request payment feature flag, go to any chat on mainnet.
Risk
Described potential risks and worst case scenarios.
Tick one: