Bug-fix: Streamlined Submit form button behavior on all forms

roshaans commented 2 years ago

This PR addresses Issue #112 to prevent the user from pressing a form button twice. It also addresses a UX bug that keeps the form button disabled until the form is valid as well as changes have been made to it. No reason to keep the form button always in the clickable state when there are not any changes to be submitted.

This behavior was changed on all flows using a form button for submission.

Solution

Take a similar approach to Near-Wallet by creating a reusable FormButton component
Take advantage of Yup Resolver's isDirty & isValid state to enable/disable form buttons
Create mutlisafe form Amount has a default value of 5.

Note: The FormButton component comes with a few handy features that we can take advantage of in future PRs such as redirecting the user to a specific screen in the app upon the button being pressed, changing the text of the button while a form is being submitted.

netlify[bot] commented 2 years ago

Deploy Preview for multisafe-near-testnet ready!

Name	Link
Latest commit	bb65439db8419add090f4f278d5a6fff338de805
Latest deploy log	https://app.netlify.com/sites/multisafe-near-testnet/deploys/62e2d1dba29ab00008fda13b
Deploy Preview	https://deploy-preview-146--multisafe-near-testnet.netlify.app/
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

roshaans commented 2 years ago

oading safe form - the button stays

Thank you for the review @marcinbodnar! I intentionally made the behaviors of the create and load multisafe forms different. I will try to address the issue for loading the mutlisafe in another PR that deals with account validation on blur. According to the logic used across the other forms, the form needs to be both in a valid state as well as changes need to be made to the form for the button to be enabled. But in its current state of the load multisafe form, the user is not given any validation of sorts if we keep the button disabled.

As for the create multisafe form, I intentionally kept the create multisafe button enabled because the user will not get any sorts of feedback from the form that they need to deposit 5 Near into the mutlisafe until they press the create button and run into the issue. If I keep the form button disabled until everything is valid in the form, then any user who is in a hurry trying to create a vault would be pretty confused as to why the button won't turn into an enabled state unless they read the fine prints under one of the fields.

I think we should by default keep the field at 5 Near. Then I can make it so I only turn the form button enabled if everything else is valid.

TLDR; In a further PR #104, I am going to address validation on Blur. This will give the user instant feedback once they step out of a field. Then, I think it makes sense to get the behavior of these forms in line with the rest of the app.

roshaans commented 2 years ago

Great job @roshaans .

Two small things:

loading safe form - the button stays available

create multi safe form - there seems to be a small delay before the button becomes disabled

Also, it would be good to add a loader and change the text in the button to Sending (or something similar). We have a main loader, but it doesn't seem to work for example on editing safe action, adding the loader in the button will help. But it can be separate PR if you prefer.

As for the loader, I think we should tackle that in another PR as well. I tried to use Yup form's isSubmitting property to change the text on the form buttons, but this is not the best method. This is because there is a delay in the app sending you to the confirm transaction screen on the near wallet, and the actual 'form submission' is much faster than that so the interaction ends up looking very awkward. I think we will need a way to keep a global state object for pending actions like there is on the Near Wallet.

roshaans commented 2 years ago

d tackle that in another PR as well. I tried to use Yup form's isSubmitting property to change the text on the form buttons, but this is not the best method. This is because there is a delay in the app sending you to the confirm transaction screen on the near wallet, and the actual 'form submission' is much faster than that so the interaction ends up looking very awkwa

U

oading safe form - the button stays

Thank you for the review @marcinbodnar! I intentionally made the behaviors of the create and load multisafe forms different. I will try to address the issue for loading the mutlisafe in another PR that deals with account validation on blur. According to the logic used across the other forms, the form needs to be both in a valid state as well as changes need to be made to the form for the button to be enabled. But in its current state of the load multisafe form, the user is not given any validation of sorts if we keep the button disabled.

As for the create multisafe form, I intentionally kept the create multisafe button enabled because the user will not get any sorts of feedback from the form that they need to deposit 5 Near into the mutlisafe until they press the create button and run into the issue. If I keep the form button disabled until everything is valid in the form, then any user who is in a hurry trying to create a vault would be pretty confused as to why the button won't turn into an enabled state unless they read the fine prints under one of the fields.

I think we should by default keep the field at 5 Near. Then I can make it so I only turn the form button enabled if everything else is valid.

TLDR; In a further PR #104, I am going to address validation on Blur. This will give the user instant feedback once they step out of a field. Then, I think it makes sense to get the behavior of these forms in line with the rest of the app.

Update: I ended up making the necessary changes to get the form behavior of load/create multisafe screens in line with the rest of the app.

agileurbanite / ui.multisafe