-
Notifications
You must be signed in to change notification settings - Fork 212
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
Tokens: refactor the side panel as a function component #1046
Conversation
And make use of useSidePanelFocusOnReady().
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.
Looks mostly good, but concerned about the removal of the max value logic.
onChange={this.handleAmountChange} | ||
min={minTokenStep} | ||
max={amountField.max} | ||
disabled={accountMaxed} |
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.
accountMaxed
, or find a different mechanism to handle it (e.g. handling it during updateAmount()
).
Right now, the form doesn't stop you from trying to assigning or removing too many tokens.
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.
Done in 5535c0b
tokenDecimals, | ||
tokenDecimalsBase, | ||
}) { | ||
const holderInputRef = useSidePanelFocusOnReady() |
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 think it should be fairly easy to do with the useSidePanel()
hook, but it would be nice if we instead focused on the amount input if the holderAddress
was already populated.
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.
Done in 5535c0b
Also: - Rename the component to UpdateTokenPanel. - Change the label to be more explicit: “Number of tokens to remove”.
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.
Just going to fix one thing with inputting decimals in the panel's amount input.
apps/token-manager/app/src/App.js
Outdated
@@ -212,5 +204,10 @@ export default () => { | |||
const theme = useTheme() | |||
const { layoutName } = useLayout() | |||
|
|||
const themeMode = useThemeMode() |
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.
Should we remove this for now?
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.
Oops this was for a local demo, it shouldn’t have been committed :o
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.
Done 03b2aaa
* TokenPanelContent: move to a function component * UpdateTokenPanel: add more warnings * Tokens: add warning if amount field is too small or has too much precision
* TokenPanelContent: move to a function component * UpdateTokenPanel: add more warnings * Tokens: add warning if amount field is too small or has too much precision
And make use of
useSidePanelFocusOnReady()
.