Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

fix(onboarding-pass-validation): added password validation #424

Merged
merged 7 commits into from Jun 28, 2018

Conversation

odb366
Copy link
Contributor

@odb366 odb366 commented Jun 15, 2018

Fixes issue #415
Now, users are not allowed to proceed with the onboarding process if their wallet pass is less than 8 chars.

@@ -35,3 +35,10 @@
visibility: visible;
}
}

.helpMessage {
Copy link
Member

Choose a reason for hiding this comment

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

currently we highlight the bottom border of the input that isn;t correct and make the border $red. we also make the error text $red. can you make those style changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we use input field with all borders making just the bottom one red will look strange. I haven't seen this pattern in sites/apps, so in my opinion it will not be consistent in usability point of view. I've made all borders and error text to get $red when error occurs. Is that Ok?

@@ -35,6 +36,9 @@ const NewWalletPassword = ({
>
Passwords do not match
</p>
<p className={`${styles.helpMessage} ${passwordMinChars && styles.visible}`}>
Copy link
Member

Choose a reason for hiding this comment

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

this visible class doesn't apply to helpMessage right? It looks like it's nested inside errorMessage in the scss file.

Also this logic seems to apply visible when the passwords match and the char count is 8 or over. Shouldn't it be the other way around?

Copy link
Member

Choose a reason for hiding this comment

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

When I'm testing this PR the error message shows up before I start typing anything

Copy link
Contributor Author

@odb366 odb366 Jun 26, 2018

Choose a reason for hiding this comment

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

The help message is intended to always stay visible for better usability. I've just forgotten to clean up the code.
If the help message is hidden some users will try to create passwords with less than 8 chars and then fail. They will be forced to rethink and resubmit their pass which is considerable user effort. We can prevent such cases by leaving the help message always visible. See this article.

@JimmyMow
Copy link
Member

@odb366 I was just testing this and added some comments/feedback

@Empact
Copy link
Contributor

Empact commented Jun 26, 2018

Could you also squash + rebase the commits?

@odb366
Copy link
Contributor Author

odb366 commented Jun 27, 2018

Thanks for the feedback guys. However, I can't commit because i got the following error. Even when I swich to my local master branch which is up-to-date i still got this error:

 webpack.config.renderer.prod.js:1:1
  ‼    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-named-as-default-member
  ‼    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-named-as-default
  ‼    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-duplicates
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/extensions
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/default
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-unresolved
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/namespace
  ×    8:34  Missing file extension for "csp-html-webpack-plugin"             import/extensions
  ×    8:34  Unable to resolve path to module csp-html-webpack-plugin.        import/no-unresolved

  webpack.config.renderer.dev.js:1:1
  ‼    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-duplicates
  ‼    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-named-as-default
  ‼    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-named-as-default-member
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/no-unresolved
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/extensions
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/default
  ×    1:1   Resolve error: Cannot find module add-asset-html-webpack-plugin  import/namespace
  ×   17:32  Unable to resolve path to module add-asset-html-webpack-plugin.  import/no-unresolved
  ×   17:32  Missing file extension for "add-asset-html-webpack-plugin"       import/extensions
  ×   18:34  Missing file extension for "csp-html-webpack-plugin"             import/extensions

@mrfelton
Copy link
Member

@odb366 you need to run yarn whenever you pull new code from github in order to install any new dependencies that might have been added.

@odb366
Copy link
Contributor Author

odb366 commented Jun 27, 2018

@mrfelton Thanks for your lightning quick answer :)

@odb366
Copy link
Contributor Author

odb366 commented Jun 27, 2018

Latest commit improves the password validation by highlighting the always visible help message Password must be at least 8 characters long in red when both passwords are of same length but less than 8 characters. This way the validation message won't appear immediately when you start typing in the Confirmation Password field.

The same way inline validation about password match is fixed in this commit. It won't appear immediately after typing in the second field, but when the users has typed equal amount of characters as in the first field.

error-message-design2

@JimmyMow
Copy link
Member

Tested ACK 1ef2c96

Nice @odb366 🎊

@Empact
Copy link
Contributor

Empact commented Jun 28, 2018

utACK 1ef2c96

@Empact Empact merged commit 58e34e2 into LN-Zap:master Jun 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants