Feature/dapp reusable action forms and progress #2921
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2921 +/- ##
==========================================
- Coverage 93.67% 92.98% -0.70%
==========================================
Files 116 203 +87
Lines 6406 8168 +1762
Branches 1157 1361 +204
==========================================
+ Hits 6001 7595 +1594
- Misses 350 478 +128
- Partials 55 95 +40
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
taleldayekh
reviewed
Aug 31, 2021
taleldayekh
reviewed
Aug 31, 2021
taleldayekh
reviewed
Aug 31, 2021
weilbith
force-pushed
the
feature/dapp-reusable-action-forms-and-progress
branch
from
7223a54 to
82862ff
Compare
|
Alright. Talel and I did a "pair-programming-review" together yesterday. The review comments above are in fact mine as I found out that the linter did not detect those unused code snippets (part of older implementation approach versions). |
taleldayekh
approved these changes
Sep 3, 2021
There was a problem hiding this comment.
Your walkthrough the other day gave me a good understanding of the changes. I skimmed through it now and could not find anything apparent that seemed out of order.
Some nit-picks about spelling but no big deal, but you should address the ones in the locales the others you can decide yourself.
Thank you! 👍🏼
raiden-dapp/tests/unit/components/channels/channel-action-form.spec.ts
Outdated
Show resolved
Hide resolved
raiden-dapp/tests/unit/components/channels/channel-action-form.spec.ts
Outdated
Show resolved
Hide resolved
This component is meant to be the unified effort of displaying the progress of an action to the user. Actions are understood as time consuming processes where the user has to wait for it to complete. Examples are opening a channel or withdrawing from the UDC. So far this is implemented everywhere multiple times over and over again with different feature sets and slightly different visuals. This component should unify them and replace them on a long term. The action progress card is just for the visualization, not the action itself. The progress consists of a overall state and the state of multiple steps that are within this action. The overall state can be in progress, completed or failed (with a respective error). Each step can be pending, active, completed or failed. Steps have a title and a description with user instructions (e.g. to confirm a blockchain transaction). An example for steps are the opening of a channel with a final deposit, where it firsts opens the channel, then does a token transfer and then deposit to the channel. This component does include no own logic to interpret data on a higher level or controls how the steps are changing their state. The usage of this component will follow soon in the next commits.
|
I now first rebased this on |
weilbith
force-pushed
the
feature/dapp-reusable-action-forms-and-progress
branch
from
7990240 to
4119851
Compare
|
Alright. I addressed all review comments. As they were all about spelling, I'm rushing a little and squash the fixup commits already. |
weilbith
force-pushed
the
feature/dapp-reusable-action-forms-and-progress
branch
from
426f3f9 to
0b6673b
Compare
|
Ah, some feedback here was actually about changes in the predecessor PR. Thereby the fixup commits do not work. I need to combine them into a new standalone commit. |
weilbith
force-pushed
the
feature/dapp-reusable-action-forms-and-progress
branch
from
0b6673b to
4ef80de
Compare
|
I suffer from the TypeScript update... |
The action progress card unifies the display of an action to the user. The now newly added action mixin is the abstract class for all action implementations. This means these components are not actually displaying anything, but control the action (steps) and use the progress card to visualize it. The first here added action which is using this setup is the channel deposit action. It is a very simple one and consists of a single step only. The mixin controls the higher level action state, and the orchestration of the control flow including the state reset and all computed properties. The parts which are individual by the actions like the steps are then customized by the components using the mixin. The mixin includes a minimal visual part which allows to either use the plain action progress card or put it into a dialog pop-up. This is helpful as in many places we trigger actions with a button and then want to visualize the progress independently from the existing layout (e.g. open channel view). In case of such a dialog it re-implements the already known feature to automatically close the dialog (with the progress) when the action has succeeded. The delay to do so can be customized. In case of a failure, the user is responsible to close the dialog himself. This also finally brings in the feature to block the user from closing the action dialog, while the action has not yet completed The mixin allows to display a slot component instead of the progress while the action is not in progress. When the action gets triggered, slot gets hidden if the progress is not showing in a dialog on top of it. Unfortunately Vue does not support single-file-components with a template for mixins. This requires to use a pure render function to provide the features of the slot and dialog handling.
This is the second action implementation. It re-uses the introduced action mixin. It is more complex than the first action to do a channel deposit as it consists of up to three steps depending on the inputs. This required some changes to the raiden service function to execute the actual channel opening. The SDK accepts a event handler function as parameter. So far this got abstracted away with a (stupid) generic function which counts the events as steps. This got removed, allowing the channel open action to directly update the actions progress steps up on the emitted events by the SDK.
This component is meant to be used for handling the parameters of a channel related action (e.g. opening a channel). It consists of the three values that relate to channel actions which are the token (address), the partner (address) and the token amount (e.g. to deposit). Each value can be either just be displayed or is allowed to be edited by the user. Values can be also completely hidden if they are clear for the context (e.g. acting on a selected channel). Inputs can be restricted according to special scenarios (e.g. exclude channel partners when opening a new channel). The form component finally provides a single component which is responsible to fetch token information and monitor them. Before (or actually still is yet) this was done everywhere we needed it. As this is an asynchronous process, this was causing a lot of issues in some components. Now they must only handle the addresses as strings and the form will do the work. The form will be bound to a specific action to execute. It also allows to customize some labeling for the specific action and some other visual adoptions. Of course it only allows to execute the action if all form values are valid. This does not apply for fixed values that are not editable. They are provided by the parent component and expected to be correct. This might be spot to improve at a later point (especially if the values come from URL parameters). The component emits events according to the outcome of the executed action. Note that for the moment the token value of the form is not editable and always fixed. This is because we don't have yet a token input component. Only the big component for the select token route. This might be added at a later point.
The channel deposit dialog could be finally drastically simplified by using the new channel deposit action and the channel action form. It basically just puts everything into a dialog and reacts to a completed deposit. This also reflects in the heavily simplified unit tests, as the tests of the action and form component test all the functionality at a single place. This commit includes the changes to adapt the dialog usage everywhere. This includes a lot of simplifications as the new unified component do the heavy lifting and the using/parent components must just use them in their templates with the correct properties. Also the end-to-end tests got marginally adopted to some new `data-cy` components and the handling of token amount input without a default zero value.
Also here we now use the new (fancy) action architecture, heavily simplifying the route component itself by using the channel action and form components. This includes improvements/refactoring of the unit tests for the route component.
|
Alright. TypeScript issues fixed and CI is fine. So I squash the fixup commits. No CHANGELOG entry required. |
weilbith
force-pushed
the
feature/dapp-reusable-action-forms-and-progress
branch
from
08a8487 to
96ca37f
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2890
Based on #2920
Short description
TBD
Definition of Done
Steps to manually test the change (dApp)