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

Don't show zero ammount message by default #2299

Merged
merged 12 commits into from
Jan 26, 2022

Conversation

nenadV91
Copy link
Contributor

@nenadV91 nenadV91 commented Jan 25, 2022

Summary

Fixes #2285

I'm not really sure that we want this change but this is what I've got.

@nenadV91 nenadV91 requested review from a team January 25, 2022 16:29
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@W3stside
Copy link
Contributor

still see it if not approved then approve. shows up
image

@nenadV91
Copy link
Contributor Author

@W3stside Updated so the approve doesn't count as touched.

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.

I'm not very convinced of the approach of keep adding stuff to the reduce state. I would think that for this, local state in the Claim/index was more than enough.

@nenadV91 nenadV91 requested a review from anxolin January 26, 2022 00:35
@nenadV91
Copy link
Contributor Author

@anxolin Good point, changed to local state. I started with different solution that required redux but after forgot that I don't need it anymore.

@elena-zh
Copy link

Looks great to me!
However, there is no generic error above the disabled 'Review' button when an investment amount is not touched..
image

I would adjust this generic error text and show it when any field on the form is in 'not touched' state.
image

WDYT?

Also, I would not show this error when the field is in the default state as well
image

@nenadV91
Copy link
Contributor Author

@elena-zh Added the generic message suggestion but kept the no balance error by default, Michel also thinks we should show this error msg and the field is disabled.

@W3stside
Copy link
Contributor

@anxolin @biocom @nenadV91 i created a simpler version of this here: #2303
may be more in line with what we want.

@elena-zh
Copy link

Thanks, @nenadV91 !
Looks much better to me.
The minor thing that was introduced in the last commit: space has become missing between token's balance and the Max button
image

Could you please fix it?

@nenadV91
Copy link
Contributor Author

@elena-zh Added Max button css fix.

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

LGTM now.
Thanks!

@@ -221,6 +223,9 @@ export default function InvestmentFlow({ hasClaims, isAirdropOnly, modalCbs }: I
))}

{hasError && <InvestFlowValidation>Fix the errors before continuing</InvestFlowValidation>}
{!hasError && someNotTouched && (
<InvestFlowValidation>Invest some amount of tokens for each option</InvestFlowValidation>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer something more generic than asking people to put money in hehe

Suggested change
<InvestFlowValidation>Invest some amount of tokens for each option</InvestFlowValidation>
<InvestFlowValidation>Required fields missing</InvestFlowValidation>

What do you think @avsavsavs

Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt of "Investment Amount is required to continue" If its too long, Then I would do "Investment Amount Required"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to Investment Amount is required to continue

@nenadV91 nenadV91 requested a review from alfetopito January 26, 2022 18:05
@nenadV91 nenadV91 merged commit 13eabc8 into release/1.10 Jan 26, 2022
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