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

UX Tweaks #40

Merged
merged 5 commits into from
Aug 26, 2021
Merged

UX Tweaks #40

merged 5 commits into from
Aug 26, 2021

Conversation

colfax23
Copy link
Contributor

Fixes #30 and working on some more for #39 .

@colfax23 colfax23 marked this pull request as ready for review August 24, 2021 00:29
@colfax23 colfax23 requested a review from rhythnic August 24, 2021 00:29
Copy link
Contributor

@rhythnic rhythnic left a comment

Choose a reason for hiding this comment

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

I haven't run it yet, but most changes look good to me. I just had a couple of suggestions.

@@ -24,14 +24,14 @@ const VerifyMnemonic: FC<VerifyMnemonicProps> = (props): ReactElement => {
return (
<Grid container direction="column" spacing={3}>
<Grid item xs={12}>
Please retype your Secret Recovery Phrase here to make sure you have it saved correctly.
Please retype your Secret Recovery Phrase here to make sure you have it saved correctly. Mistakes could cause a loss of funds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Saying "mistakes could cause a loss of funds" at this point is a little confusing, because it's a validation step and there are no funds yet, and mistakes would prevent the user from continuing. I think the "mistakes..." text would be better in the 2-GenerateMnemonic step just prior to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - done!

<Grid item xs={2}>
{!props.hideNext && (
{!props.hideNext && !props.centerNext && (
Copy link
Contributor

Choose a reason for hiding this comment

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

In cases where there is only one button and it should be centered, I think it would be better to not use the StepNavigation component, and just use a centered button instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done!

Copy link
Contributor

@rhythnic rhythnic left a comment

Choose a reason for hiding this comment

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

👍

@colfax23 colfax23 merged commit 212b855 into stake-house:main Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Center 'Finish' button on final page
2 participants