Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Show warning when invested amount is not maximum available amount #2288

Merged
merged 11 commits into from
Jan 26, 2022

Conversation

nenadV91
Copy link
Contributor

Summary

If the invested amount is not equal to maximum available amount we show yellow warning text below the input field

Screenshot from 2022-01-25 12-20-52

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@nenadV91 nenadV91 requested review from a team January 25, 2022 11:56
@elena-zh
Copy link

elena-zh commented Jan 25, 2022

Hey @nenadV91 , great changes!
A tiny nitpick: it shows 100% of the investment amount, but actually it is not 100%. Could we show 99,99% instead?
image
image

Video: https://watch.screencastify.com/v/26mQjbcnGBVjHkz3raJt

Also, as I mentioned in #2279 (comment) , it would be nice to change warning's color for the light mode, I think.


const ErrorMsgs = {
const Messages = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why change the name here, they are more declarative as ErrorMessage no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because its also warnings and not just errors at this point. What do you think?

@alfetopito
Copy link
Contributor

Hey @nenadV91 , great changes! A tiny nitpick: it shows 100% of the investment amount, but actually it is not 100%. Could we show 99,99% instead?

I don't think such a tiny amount (less than 0.01%) is relevant. I vote for leaving as is.

Also, as I mentioned in #2279 (comment) , it would be nice to change warning's color for the light mode, I think.

That will come, @biocom will get there 🤞

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Works great but once concern: we should either move this warning up (so it triggers first) or make a list of warnings

I say that because the native fee warning will hide this one:
Screen Shot 2022-01-25 at 09 17 37

@nenadV91 nenadV91 requested a review from alfetopito January 25, 2022 17:49
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Nice! all good other than what Leandro mentioned.

make a list of warnings

I would go for that one. For the errors, with one we have enough I believe. But Warnings we should see all the warnings at the same time.

Maybe @annamsgeorge or @avsavsavs want to touch that message. It's used for warning the users when they only make use of part of their claiming opportunity.

@nenadV91
Copy link
Contributor Author

@anxolin Updated to an array of warnings

@nenadV91 nenadV91 requested a review from anxolin January 25, 2022 21:19
@fairlighteth
Copy link
Contributor

@nenadV91 Pushed a fix for the style.ts merge conflict, in case it helps.

@elena-zh
Copy link

Hey, now I see not readable warning messages in the dark mode:
image

@nenadV91
Copy link
Contributor Author

@elena-zh Good catch, my mistake, fixed! Maybe later on @michelbio can also take a look at this.

@elena-zh
Copy link

@nenadV91 , thanks!
Now it is readable. But let wait for @biocom for the final decision on the text colors.

@fairlighteth
Copy link
Contributor

Can review post merge to address it then.

@nenadV91 nenadV91 merged commit 4019ea5 into release/1.10 Jan 26, 2022
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Cool

@@ -368,7 +400,15 @@ export default function InvestOption({ claim, optionIndex, openModal, closeModal
</i>
{/* Insufficient balance validation error */}
{inputError && <small>{inputError}</small>}
{inputWarning && <small className="warn">{inputWarning}</small>}
{inputWarnings.length ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not this the same as

{ inputWarnings.length && (... ) }

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

Successfully merging this pull request may close these issues.

6 participants