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

Enhance the management of 'confirm' button state in components that use TokenSelector & update copy #3151

Merged
merged 6 commits into from
Feb 8, 2022

Conversation

danbr
Copy link
Contributor

@danbr danbr commented Jan 31, 2022

Description

This PR adds corrects the behavior of TokenEditDialog, and updates the copy to read "Token data not found. Please check the token contract address.".

A new callback prop to TokenSelector was added to inform the caller of when an address is being searched.

Initially, TokenEditDialog was enhanced to guard the 'confirm' button when performing verification & validation. But 2 other components (UserTokenEditDialogForm & StepSelectToken) that use TokenSelector also had the same issues, so they were also updated.

Before updates:

Screenshot 2022-01-30 at 17 43 34

After updates:

Screenshot 2022-01-30 at 17 46 54

Screenshot 2022-01-30 at 17 41 01

Screenshot 2022-01-30 at 17 47 08

Screenshot 2022-01-30 at 17 47 18

Resolves #3121

@danbr danbr added the bug label Jan 31, 2022
@danbr danbr requested a review from a team January 31, 2022 01:16
@danbr danbr self-assigned this Jan 31, 2022
Copy link
Contributor

@alicjakujawa alicjakujawa left a comment

Choose a reason for hiding this comment

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

I would get rid of this repeated code. I would make use of "onTokenSelect" callback, as if it was fired && it has token data - it means that its not "loading" anymore (or is not "checking" anymore)

@danbr danbr requested a review from a team February 1, 2022 03:39
Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

Looks to be working great.

One minor suggestion, we could probably change this status indicator to be red on the "Token data not found..." error message.

error-message

Copy link
Contributor

@chinins chinins left a comment

Choose a reason for hiding this comment

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

Looks good, I have just one doubt about maintaining same states (hasError) in 2 components (as per my comment directly in the file)

@danbr danbr requested a review from a team February 2, 2022 23:16
Copy link
Contributor

@chinins chinins left a comment

Choose a reason for hiding this comment

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

I think this looks better now, with single source of truth for both the parent & the child.

@danbr danbr changed the title Enhance the management of 'confirm' button state in components that use TokenSelector, and update copy. Enhance the management of 'confirm' button state in components that use TokenSelector & update copy Feb 3, 2022
Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a comment

Choose a reason for hiding this comment

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

Nice! I quite like these changes 👌

@danbr danbr force-pushed the bug/3121-manage-token-button-state branch from 48bba1c to f009a69 Compare February 8, 2022 21:28
@danbr danbr merged commit 10cccc9 into master Feb 8, 2022
@danbr danbr deleted the bug/3121-manage-token-button-state branch February 8, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix button state while loading and the error message in "Manage tokens" dialog
5 participants