Add a password strength component #4153

ngotchac · 2017-01-12T15:13:33Z

Add it to the account creation modals, and to the change password modal

jacogr · 2017-01-12T15:38:54Z

Would prefer https://github.com/ethcore/parity/pull/3988 in first for the creation part since it reworks everything into a store. (Need to get through 1.5 release first before final checks on that one)

ngotchac · 2017-01-12T16:14:29Z

@jacogr Well it's only a new Component that adds one line to each Accounts render, so I guess it wouldn't be affected by this #3988 update

jacogr · 2017-01-12T16:36:13Z

The branches are not a clean merge at all, it has been completely re-done. (I18n, imports, formatting, store)

Alternatively don't touch createAccount here and just do change as step 1.

jacogr · 2017-01-12T16:37:41Z

js/src/ui/Form/PasswordStrength/passwordStrength.js

+};
+
+export default class PasswordStrength extends Component {
+


Extra unneeded line.

jacogr · 2017-01-12T16:38:46Z

js/src/ui/Form/PasswordStrength/passwordStrength.js

+      <div className={ styles.strength }>
+        <label className={ styles.label }>
+          <FormattedMessage
+            id='passwordStrength.label'


prefer ui.passwordStrength.label (top-level names reserved for views/modals, there is a lot of overlap with component-names)

jacogr · 2017-01-12T16:39:50Z

js/src/ui/Form/PasswordStrength/passwordStrength.js

+  }
+
+  // Note that the suggestions are in english, thus it wouldn't
+  // make sense to add translations to surrounding words


We cannot be 99.9% and once in we shouldn't need to come back. What are the alternatives?

To clarify the above - at the very least all our text should be i18n. (3rd party could be non - for now). So the below should either just display the library hint or split it such that our part is translatable and their part a variable input.

(This has been taken care of)

jacogr · 2017-01-12T16:40:20Z

js/src/ui/Form/PasswordStrength/passwordStrength.js

+        return 'orange';
+
+      default:
+      case 0:


case 0: default:

(Or remove 0)

jacogr · 2017-01-12T16:41:12Z

js/src/ui/Form/PasswordStrength/passwordStrength.spec.js

+  describe('rendering', () => {
+    it('renders', () => {
+      expect(render({ input: INPUT_A })).to.be.ok;
+      expect(render({ input: INPUT_A }).find('LinearProgress')).to.be.ok;


Prefer seperate it blocks, otherwise you need to go by line to find the failure - one tests the component, the second parts of the component.

jacogr · 2017-01-12T16:41:55Z

js/src/ui/Form/PasswordStrength/passwordStrength.js

+import React, { Component, PropTypes } from 'react';
+import { debounce } from 'lodash';
+import { LinearProgress } from 'material-ui';
+import { FormattedMessage } from 'react-intl';


While we are in this file and it is new, alphabetical order.

Well it is, except for React which is the main dependency of this module, I don't think it's necessarily bad to have it on top.

Would have expected at least the react-releated stuff to follow react, hence making exceptions jumbles it up and seems random. i.e. we typically do (not the case here)

import React from 'react'; import { FormattedMessage } from 'react-intl';

(grouped above, easy to see relations)

Here React is the exception, the rest sorted so it is a mix-match. Small thing (and driving me crazy), but like with everything, allows us to not have to think "in this case we do it differently".

Not a crisis, still worth being consistent about.

ngotchac · 2017-01-12T18:06:00Z

Okay, fixed grumbles and removed the PasswordStrength component from the New Accounts modal.

coveralls · 2017-01-12T18:55:58Z

Changes Unknown when pulling 74dc32d on ng-password-strengh into ** on master**.

jacogr · 2017-01-13T09:18:00Z

@gavofyork on your B0 -

As this PR stands now it only covers the change area, hence not everywhere as it stands
To get it covered, @ngotchac would need to revert some of his latest changes - simple enough (If we desperately want this in, should do that, even it is going to create issues for us backporting future 1.5/1.6 fixes related to this)
Generally I prefer not putting in last-minute functionality changes when testing is underway, but up to you

* Added new PasswordStrength Component * Added tests * PR Grumbles

* verification: check if server is running (#4140) * verification: check if server is running See also ethcore/email-verification#67c6466 and ethcore/sms-verification#a585e42. * verification: show in the UI if server is running * verification: code style ✨, more i18n * fix i18n key * Optimized hash lookups (#4144) * Optimize hash comparison * Use libc * Ropsten fork detection (#4163) * Stop flickering + added loader in AddressSelector (#4149) * Stop UI flickering + added loader to AddressSelector #4103 * PR Grumbles * Add a password strength component (#4153) * Added new PasswordStrength Component * Added tests * PR Grumbles * icarus -> update, increase web timeout. (#4165) * icarus -> update, increase web timeout. * Fix estimate gas * Fix token images // Error in Contract Queries (#4169) * Fix dapps not loading (#4170) * Add secure to dappsreg * Remove trailing slash // fix dapps

ngotchac added 2 commits January 12, 2017 16:03

Added new PasswordStrength Component

e875a9c

Added tests

c2c57a8

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 12, 2017

jacogr reviewed Jan 12, 2017

View reviewed changes

js/src/ui/Form/PasswordStrength/passwordStrength.js

return 'orange';

default:

case 0:

Copy link

Contributor

jacogr Jan 12, 2017

Choose a reason for hiding this comment

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

case 0: default:

(Or remove 0)

jacogr reviewed Jan 12, 2017

View reviewed changes

PR Grumbles

74dc32d

gavofyork added the B0-patch label Jan 13, 2017

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 13, 2017

jacogr merged commit 4a714d4 into master Jan 13, 2017

jacogr deleted the ng-password-strengh branch January 13, 2017 14:52

arkpar pushed a commit that referenced this pull request Jan 16, 2017

Add a password strength component (#4153)

7429239

* Added new PasswordStrength Component * Added tests * PR Grumbles

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a password strength component #4153

Add a password strength component #4153

ngotchac commented Jan 12, 2017

jacogr commented Jan 12, 2017

ngotchac commented Jan 12, 2017

jacogr commented Jan 12, 2017 •

edited

Loading

jacogr Jan 12, 2017

jacogr Jan 12, 2017

jacogr Jan 12, 2017

jacogr Jan 12, 2017

ngotchac Jan 13, 2017

jacogr Jan 12, 2017

jacogr Jan 12, 2017

jacogr Jan 12, 2017

ngotchac Jan 12, 2017

jacogr Jan 13, 2017

ngotchac commented Jan 12, 2017

coveralls commented Jan 12, 2017

jacogr commented Jan 13, 2017 •

edited

Loading

Add a password strength component #4153

Add a password strength component #4153

Conversation

ngotchac commented Jan 12, 2017

jacogr commented Jan 12, 2017

ngotchac commented Jan 12, 2017

jacogr commented Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac commented Jan 12, 2017

coveralls commented Jan 12, 2017

jacogr commented Jan 13, 2017 • edited Loading

jacogr commented Jan 12, 2017 •

edited

Loading

jacogr commented Jan 13, 2017 •

edited

Loading