-
Notifications
You must be signed in to change notification settings - Fork 46
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
Issue/75 address input for known address #79
Conversation
@@ -571,9 +570,8 @@ const Show = () => { | |||
<Label><Hint>To</Hint></Label> | |||
<AddressInput | |||
addressValue={transferTo} | |||
setAddressCallback={(value) => setTransferTo(value)} | |||
setAddressCallback={setTransferTo} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that cause a warning "modifying another component's state inside a component"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this warning. But it is a call back to set the value in this component, and this component's value is being supplied as prop to the AddressInput
component.
Looks good, but I am getting
|
I will double check and fix it. |
|
||
return ( | ||
<Select.Option key={index} value={util.safeOneAddress(knownAddress.address)}> | ||
<Space size='small' align='baseline'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
justify='spaceBetween'
? Looks weird when checkmarks are unaligned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I did not what to set there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it will need a Row
and it is space-between
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought it would be Grid
layout. I will play around it and try to make it align.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with Grid layout and removed the check button, replaced with text button that spans the whole Col space.
It is reporting
|
numUsed: 0 | ||
})) | ||
}) | ||
}, [knownAddresses, wallets, dispatch]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the issue is with this dependency knownAddresses
. There might be some invalid wallets left in the user's state (e.g. my browser). If the invalid wallet has no valid address
, it would become part of walletsNotInKnownAddresses
and sent to dispatch for setKnownAddress
. The reducer in setKnownAddress
can't add that invalid wallet's address to known addresses, but would create a new object for knownAddresses
, therefore trigger another useEffect
here, hence causing an infinite loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made this effect run once when the component load.
showSelectManualInputAddress | ||
? ( | ||
<Select.Option key='address-value' value={addressValue.value}> | ||
<Space size='small' align='baseline'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onClick needs to be on Space, otherwise the function won't trigger
block | ||
type='text' | ||
style={{ textAlign: 'left' }} | ||
onClick={() => onSelectAddress({ value: addr, label: longAddressLabel, key: index })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as below
<Space size='small' align='baseline'> | ||
{addressValue.value} | ||
<Button type='text' onClick={() => onSelectAddress(addressValue)}> | ||
<CheckOutlined /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer needed
code/client/src/pages/Create.jsx
Outdated
<Space size='small' align='baseline'> | ||
I want to do this later in my wallet | ||
<Button type='text' onClick={() => setLastResortAddress({ value: '', label: 'I want to do this later in my wallet' })}> | ||
<CheckOutlined /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer needed, but since we are removing this option altogether - you can update it in the next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, forgot to remove it, will remove and push.
code/client/src/pages/Create.jsx
Outdated
extraSelectOptions={ | ||
[ | ||
<Select.Option key='later' value=''> | ||
<Space size='small' align='baseline'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Looks good |
No description provided.