Skip to content
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

Merged
merged 5 commits into from
Nov 27, 2019
Merged

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented Nov 22, 2019

And make use of useSidePanelFocusOnReady().

And make use of useSidePanelFocusOnReady().
Copy link
Contributor

@sohkai sohkai left a 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}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ We should keep the logic for 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.

Copy link
Contributor Author

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()
Copy link
Contributor

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.

Copy link
Contributor Author

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”.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.841% when pulling 5535c0b on panel-function into e614d3b on master.

Copy link
Contributor

@sohkai sohkai left a 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.

@@ -212,5 +204,10 @@ export default () => {
const theme = useTheme()
const { layoutName } = useLayout()

const themeMode = useThemeMode()
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 03b2aaa

@sohkai sohkai merged commit 90ad353 into master Nov 27, 2019
@sohkai sohkai deleted the panel-function branch November 27, 2019 00:01
MickdeGraaf pushed a commit to MickdeGraaf/aragon-apps that referenced this pull request Jan 28, 2020
* TokenPanelContent: move to a function component
* UpdateTokenPanel: add more warnings
* Tokens: add warning if amount field is too small or has too much precision
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
* TokenPanelContent: move to a function component
* UpdateTokenPanel: add more warnings
* Tokens: add warning if amount field is too small or has too much precision
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants