[Claim refactor] - Split InvestmentFlow and minor slider impl. #2144
Conversation
|
| } | ||
|
|
||
| const handleStepChange = (value: number) => { | ||
| console.log(value) |
There was a problem hiding this comment.
one thing here, if we decide to use this, is to make sure to debounce this
|
|
||
| const amount = maxCost.greaterThan(balance) ? balance : maxCost | ||
| // store the value as a string to prevent unnecessary re-renders | ||
| const investAmount = formatUnits(amount.quotient.toString(), balance.currency.decimals) |
There was a problem hiding this comment.
why aren't you saving it as atoms? (may be @alfetopito who did this) but question remains
especially since you're using it in other places to do maths and need to reconvert. We usually just formatSmart the thing from atoms or am I wrong?
There was a problem hiding this comment.
Because it matches the input value, which won't be in atoms.
I was using atoms at first, but then the input start behaving very weird.
| return | ||
| } | ||
|
|
||
| const investA = CurrencyAmount.fromRawAmount(token, parseUnits(investedAmount, token.decimals).toString()) |
There was a problem hiding this comment.
like here investedAmount could just be passed as a string from state (if it were saved as atoms)
There was a problem hiding this comment.
Yep, I was doing that before, but then I would need to convert this in other places anyway.
The biggest problem is the input
| <b>Token approval</b> | ||
| <i> | ||
| {approveState === ApprovalState.NOT_APPROVED ? ( | ||
| `${currencyAmount?.currency?.symbol} not approved` |
There was a problem hiding this comment.
yeah i think we should create a type checking
function isDefinedCurrency(currency: Currency | undefined) currency is Required<Currency> {
...
}to help remove these optional params hell
There was a problem hiding this comment.
( I also didn't like adding another prop just for the name but maybe we should add that back in )
There was a problem hiding this comment.
Nah, I have that on top as a nice and single symbol variable.
But I do like the idea of Required
src/custom/pages/Claim/index.tsx
Outdated
| @@ -23,7 +23,7 @@ import ClaimAddress from './ClaimAddress' | |||
| import CanUserClaimMessage from './CanUserClaimMessage' | |||
| import ClaimingStatus from './ClaimingStatus' | |||
| import ClaimsTable from './ClaimsTable' | |||
| import InvestmentFlow from './InvestmentFlow' | |||
| import InvestmentFlow from './Investment' | |||
There was a problem hiding this comment.
Why not calling the dir also InvestmentFlow?
There was a problem hiding this comment.
i did this, was tired, my bad
src/custom/pages/Claim/index.tsx
Outdated
| @@ -175,7 +175,7 @@ export default function Claim() { | |||
| // on account change | |||
| useEffect(() => { | |||
| if (!isSearchUsed && account) { | |||
| setActiveClaimAccount(account) | |||
| setActiveClaimAccount('0xD5e900280Eb1aDe4E583c2bF2414be247E298435') | |||
There was a problem hiding this comment.
Fixed, just for development so I don't have to insert one while working on investment flow
|
|
||
| <div> | ||
| <RangeSteps> | ||
| {[0, 25, 50, 75, 100].map((step: number) => ( |
There was a problem hiding this comment.
this array should probably just be created outside the body
There was a problem hiding this comment.
You can leave this part out, there's a component (semi-)ready coming from Protofire
|
|
||
| const updateInvestAmount = useCallback( | ||
| (idx: number, investedAmount: string) => { | ||
| console.log(idx, investedAmount) |
There was a problem hiding this comment.
make TODO to remove and/or change log to be more descriptive plz
| ))} | ||
|
|
||
| <InvestTokenSubtotal> | ||
| {activeClaimAccount} will receive: 4,054,671.28 vCOW based on investment(s) |
There was a problem hiding this comment.
add TODO to change this
…ap into claim-split-investment-flow
| </span> | ||
| <label> | ||
| <b>{currencyAmount?.currency?.symbol}</b> | ||
| <input disabled placeholder="0" value={investedAmount} max={formatSmart(currencyAmount)} /> |
There was a problem hiding this comment.
Actually the max might be different than currencyAmout. Maybe we could export the logic from onMaxClick to be re-used here
|
to be merged in this state - next PRs will tackle logic changes. |
| } | ||
|
|
||
| const amount = maxCost.greaterThan(balance) ? balance : maxCost | ||
| // store the value as a string to prevent unnecessary re-renders |
There was a problem hiding this comment.
This might be a dumb reason and not really apply, as we depend on other stuff that are object instances.
I started with this but did not get around to clear it.
One reason to keep it though is to be closer to the input, not a lot of conversions on every keystroke
Splits InvestmentFlow and implements barebones slider implementation (for another PR)
@nenadV91 please take over and finish this one, keep it small please!