Conversation
| events: assign({}, FormView.prototype.events, { | ||
| 'click .use-different': preventDefaultThen('useDifferentAccount'), | ||
| 'keyup #vpassword': '_onConfirmPasswordKeyUp', | ||
| 'blur #vpassword': '_onConfirmPasswordBlur', |
There was a problem hiding this comment.
I added check on blur because if user tabs to age input and the delay hasn't check it, it will automatically show the tooltip error. It seemed to be a little smoother.
| const t = msg => msg; | ||
|
|
||
| const PASSWORD_INPUT_SELECTOR = '#vpassword'; | ||
| const DELAY_BEFORE_PASSWORD_CHECK_MS = 2000; |
There was a problem hiding this comment.
@ryanfeeley 2 seconds seem reasonable to me while manually testing. Any objections?
vbudhram
force-pushed
the
issue-631
branch
2 times, most recently
from
60467d1 to
9bd8aca
Compare
|
@mozilla/fxa-devs I think this is ready! r? |
| _onConfirmPasswordKeyUp() { | ||
| this.checkPasswordsMatchDebounce(); | ||
| }, | ||
|
|
||
| _onConfirmPasswordBlur() { | ||
| this._checkPasswordsMatch(); | ||
| }, |
There was a problem hiding this comment.
Does this mean the check is performed twice when I tab out of the confirm password field? Once on the blur, and then again DELAY_BEFORE_PASSWORD_CHECK_MS after the last debounce call?
There was a problem hiding this comment.
Hmm yea, it does. Let me add a few checks to make this work a little smoother.
|
@chenba made the updates! |
|
@davismtl is this something you would like to get on train 150? |
|
|
||
| _onConfirmPasswordKeyUp() { | ||
| if (!passwordCheckLock) { | ||
| passwordCheckLock = true; |
There was a problem hiding this comment.
This changes the previous debounce behavior. IIRC previously debounce is called on every keyup. With this extra state, debounce is called on the first keyup, then again on the first keyup after DELAY_BEFORE_PASSWORD_CHECK_MS. If the blur handler executes during the delay, it won't perform the check since passwordCheckLock will be true.
Is this intentional?
There was a problem hiding this comment.
Yea this was intentional because otherwise you would see a slight flicker on the tooltip when called twice.
vbudhram
force-pushed
the
issue-631
branch
2 times, most recently
from
19e16cf to
479c7e5
Compare
|
So on further investigating and testing, I am opting to remove the password match check on In practice it works wells and I don't see a huge difference. The second password field will get focused as part of the debounced function. Keeping the original behavior did not play well with the first password field (you couldn't focus the input to change it). I think we can come up with a more general solution that will work for both password fields as part of the work @LZoog is doing. cc @davismtl @mozilla/fxa-devs r? |
2 participants
Fixes #631