-
Notifications
You must be signed in to change notification settings - Fork 82
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
frontend: improvements of Send
component
#2966
Open
NicolaLS
wants to merge
8
commits into
BitBoxSwiss:master
Choose a base branch
from
NicolaLS:final-improvments-before-refactor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
frontend: improvements of Send
component
#2966
NicolaLS
wants to merge
8
commits into
BitBoxSwiss:master
from
NicolaLS:final-improvments-before-refactor
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
NicolaLS
changed the title
Final improvments before refactor
frontend: improvements of Oct 8, 2024
Send
component
Utilize the `SendWrapper` to get the mobile channel error, and (bb01) paired status in case a bb01 is connected. This simplifies the `Send` component and better distinguishes code for bb01/bb02 stuff.
Type `coinUnit` in `IAccount` to use `CoinUnit`.
Remove the unused `account` state from the `State` type declaration because it is never used. Remove the `paired?` state from the `Send` `State` declaration and also remove its usage in the return statement. It is always `undefined` which means `hidden={paired !== false}` is always `true`. This means we can remove it completely.
The `txProposalErrorHandling` function should **only** return errors and not set any state. Although this isn't causing an issue right now, the function should not return an object that is unrelated to errors in principle. Additionally, this will be necessary for an upcoming refactor of the `Send` component. Return only `TProposalError` from the function but make all fields optional. To keep the logic the same we set `proposedFee` to `undefined` according to the same logic (ie. `{}` or `{ amountError: ... }` is returned) but in the `Send` component not in the error handling function.
Instead of individual variables `proposedFee`, `proposedTotal`, and `proposedAmount` we can put the result itself into the component state. Because of this, it is not possible anymore to reset individual fields to `undefined` but this is correct since there should be no "old" proposal values available in case the current proposal was invalid resulting in an `undefined` `proposalResult`. Before this commit the amount fields could show false values because they would show the last converted `fiatAmount` and the last valid `proposedAmount` since it was not reset on error. Now that we reset it it would've shown `''` but this is not a good solution which is why this commit also falls back to the last typed amount and its fiat equivalent in case `sendAll` is `true` but the proposal was invalid. We also disable the `sendAll` checkbox in case there is an `addressError` or `feeError` because it can not work if the proposal is not valid. We don't disable it if there's only `amountError` because the `amount` is not relevant if `sendAll` is `true`.
Set the `isUpdatingProposal` to `false` in the `finally` block of `validateAndDisplayFee` instead of twice in the `txProposal` funciton. This makes the component a bit easier to understand and smaller.
Improve the onChange handler of the `NoteInput` component to take a function that takes a string instead of the `ChangeEvent`. This will enable us to just pass the `setNote` setter to it when we refactor the `Send` component to be functional.
Don't reset the custom fee when the fee target changes/is set. The backend only uses the custom fee if fee target = custom which means it does not need to be set to be empty (''). Like this the user can select custom, type something, change to some other target, but as soon as he chooses custom again, what he typed earlier is still there. This will also enable us to only pass a `setFeeTarget` to the sub-component instead of having to define a handler, later when we refactor the `Send` component to functional.
NicolaLS
force-pushed
the
final-improvments-before-refactor
branch
from
October 14, 2024 15:14
9c67653
to
d437c29
Compare
Is it ready to be reviewed? :) @NicolaLS |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
After #2955
Refactoring the
Send
component I noticed these as either improvements or required preparation for the refactor. The commits are quite small and I don't think they need to be tested individually, just test HEAD of the PR, I can also squash them all into one "Send component improvements" commit ?Please review each commit individually.
Putting the result of the proposal request into the state changes the most:
be190c6
But it also fixes a bug that exists on master right now, to reproduce:
After this commit there will never be any
proposedStuff
in case the last proposal (and thus partially our current state) is invalid. To prevent this the "send all" checkbox is disabled on address/fee errors. And we fallback on the last typed amount/fiat in case we end up in the state ofsendAll = true
butproposalResult = undefined
.In case you think we should reset the custom fee I can drop this:
9c67653
But it will let us remove the handler function later and only pass the state setter when we refactor
Send
to functional in case we don't reset the custom fee.