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

Claim investment flow 2 #2163

Merged
merged 8 commits into from
Jan 18, 2022
Merged

Claim investment flow 2 #2163

merged 8 commits into from
Jan 18, 2022

Conversation

nenadV91
Copy link
Contributor

Summary

  • Moved investment flow data to redux
  • Added implementation for progress par percentages

@nenadV91 nenadV91 requested a review from a team January 15, 2022 19:23
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

// investing
setIsInvestFlowActive: (payload: boolean) => void
setInvestFlowStep: (payload: number) => void
initInvestFlowData: (payload: EnhancedUserClaimData[]) => void
Copy link
Contributor

@W3stside W3stside Jan 16, 2022

Choose a reason for hiding this comment

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

dont save complex shapes to redux state, it's unoptimised, and we can't debug them in redux dev tools.

in this case EnhancedUserClaimData is:

type EnhancedUserClaimData = UserClaimData & {
  currencyAmount?: CurrencyAmount<Token | GpEther> | undefined
  claimAmount?: CurrencyAmount<Token> | undefined
  price?: Price<Currency, Currency> | undefined
  cost?: CurrencyAmount<Currency> | undefined
  isFree: boolean
}

do we really need all of that? if not only save what we need and only save the string representations of what we need. i realy dont know if we do need this in state tho...

happy to hear otherwise

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 with simple redux data structure with just index and investAmount

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.

My biggest concern with this PR is that it looks like duplicates the work Protofire did #2060

Why not using https://pr2060--gpswapui.review.gnosisdev.com/#/profile ?

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx Outdated Show resolved Hide resolved
@fairlighteth
Copy link
Contributor

@nenadV91 Committed a small merge conflict fix, involving the styled.ts file.

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.

Approved, although mainly to get it merged and be able to reiterate

setPercentage(value)
},
[balance, maxCost, optionIndex, updateInvestAmount]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

as we discussed yesterday, we won't need this (unles u want it for set max amount), which i guess would be overkill

@@ -194,7 +209,7 @@ export default function InvestOption({ approveData, updateInvestAmount, claim }:
<input
// disabled
placeholder="0"
value={investedAmount}
value={investedAmount ? formatUnits(investedAmount, decimals) : '0'}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this field be handled if it changes?
It can be done in another PR

updateInvestAmount(claim.index, investAmount)
}, [balance, claim.index, maxCost, updateInvestAmount])
updateInvestAmount({ index: optionIndex, amount: amount.quotient.toString() })
setPercentage(INVESTMENT_STEPS[INVESTMENT_STEPS.length - 1])
Copy link
Contributor

Choose a reason for hiding this comment

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

what a elaborate way to just set the percentage to 100%

Copy link
Contributor

Choose a reason for hiding this comment

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

hehe that's my bad :P we did this together on the call

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.

The max investment and amount when i select 100% doesn't match

image

Also, the input is not editable

@nenadV91 nenadV91 merged commit 45c0b23 into claim Jan 18, 2022
@alfetopito alfetopito deleted the claim-investment-flow-2 branch January 18, 2022 19:37
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.

4 participants