Skip to content
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

regression bug fixing #1680

Merged
merged 1 commit into from
Aug 31, 2021
Merged

regression bug fixing #1680

merged 1 commit into from
Aug 31, 2021

Conversation

bassemmagdy
Copy link
Contributor

@bassemmagdy bassemmagdy commented Aug 30, 2021

@sonarcloud
Copy link

sonarcloud bot commented Aug 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -602,6 +602,7 @@ export class FormTransferTransactionTs extends FormTransactionBase {
@Watch('selectedSigner')
onSelectedSignerChange() {
this.formItems.signerAddress = this.selectedSigner.address.plain();
this.resetForm();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this break this change? => #1642
Should we just update the necessary parts instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, it wouldn't break it. After having this sticky account feature it works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sending from multisig "From" selector stays selected on previously choosen multisig.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say as a user, I filled the transfer form and then noticed that I've chosen the wrong account, currently, all the other fields remain the same when I changed the signer but with this change, I'll have to re-enter all the information since it'll reset the form, right? I'm not sure if that's the behaviour we want to have in the transfer form tbh, @cryptoBeliever @coiki any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we need the form to be reactive especially when it comes to the held mosaics in each account, I believe we need to reset fields on the change of signer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yilmazbahadir you are right. I thought about other case here.
@bassemmagdy would be possible to refresh only mosaics?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can only reset the held mosaics (dropdown and maybe the amount - unsure) rather than resetting the form, right? (If it won't break the reactivity ofc)

#1680 (comment)

Should we just update the necessary parts instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Form is cleaned after tx confirm. So we have regression with this fix -> #1577

Copy link
Contributor

@cryptoBeliever cryptoBeliever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aggregate complete - invalid date of send transaction Multisig account transfer view - mosaics not visible
4 participants