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
Show/Hide SSN field (LG-3641) #4342
Show/Hide SSN field (LG-3641) #4342
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
app/javascript/app/ssn-field.js
Outdated
|
||
input.maxLength = 9; | ||
} else { | ||
input.maxLength = 11; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe drop a comment it's 9 + 2 dashes?
3d937a5
to
ed96d0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bpdesigns @zachmargolis @aduth The tests were kind enough to discover that we should probably set the max length to 11 always, in case a user is typing in their own dashes. We have some error messages suggesting that format as well: identity-idp/config/locales/idv/en.yml Line 26 in 21be6a3
|
c3aee16
to
a2939ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, but we should fix the error when toggling the checkbox (last review comment).
app/javascript/app/ssn-field.js
Outdated
|
||
if (!toggle.checked) { | ||
if (ssnCleave) { | ||
ssnCleave.destroy(); | ||
} | ||
|
||
input.value = input.value.replace(/-/g, ''); | ||
} else { | ||
ssnCleave.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to destroy the current instance regardless of condition, we could pull it out as common. Could also use optional chaining to avoid checking if it exists.
if (!toggle.checked) { | |
if (ssnCleave) { | |
ssnCleave.destroy(); | |
} | |
input.value = input.value.replace(/-/g, ''); | |
} else { | |
ssnCleave.destroy(); | |
ssnCleave?.destroy(); | |
if (!toggle.checked) { | |
input.value = input.value.replace(/-/g, ''); | |
} else { |
app/javascript/app/ssn-field.js
Outdated
toggle.addEventListener('change', function () { | ||
input.type = toggle.checked ? 'text' : 'password'; | ||
|
||
if (!toggle.checked) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: With if
else
, I find it's less confusing if the if
condition isn't negated ("if [x] else [not x]" rather than "if [not x] else [not not x]").
if (!toggle.checked) { | |
if (toggle.checked) { |
(more changes necessary than suggestion indicates)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah +1 I'm a big fan of using the positive (not-negated) case first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
app/javascript/app/ssn-field.js
Outdated
|
||
input.value = input.value.replace(/-/g, ''); | ||
} else { | ||
ssnCleave.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the previous comment about optional chaining, it also appears that on initial load, since the field is hidden, there is no ssnCleave
instance, so this errors when toggling the checkbox.
Uncaught TypeError: Cannot read property 'destroy' of undefined
The optional chaining would resolve this. But there's another point as well, which is that we're making assumptions about the default state, and assuming that cleave only needs to be initialized on change. But strictly speaking, it needs to be initialized any time the field is visible. So in a future scenario where we'd decide to show the text by default, we'd not be initializing it.
There's some prior art at #4157 to addressing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hacked around a bit and am in new territory, and I'm not sure if I'm handling the potential for multiple inputs correctly, but I added daf511f to address the initialization and syncing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. We could also inline the function definition of sync
so it has direct access to the variables in the outer scope, in order to avoid passing as arguments.
let ssnCleave;
function sync() {
input.type = toggle.checked ? 'text' : 'password';
input.value = input.value.replace(/-/g, '');
ssnCleave?.destroy();
ssnCleave = toggle.checked
? new Cleave(input, {
numericOnly: true,
blocks: [3, 2, 4],
delimiter: '-',
})
: null;
}
toggle.addEventListener('change', sync);
sync();
I like the changes here to avoid having the password toggle apply automatically and instead require opt-in. Alternatively, if we didn't want to do that, we could keep the existing behavior, and have |
The show/hide copies functionality from
pw-toggle.js
, and moves the previousCleave
functionality as well.To keep the functionality consistent in only allowing numbers, the SSN field adds and removes different Cleave options and changes the max length of the field to accommodate dashes. The dashes are removed in the hidden state to avoid adding characters, which could mislead users about how many characters they'd entered.
pw-toggle.js
automatically added "Show password" to every password input, which conflicted with using password for the SSN field, so I added thepassword-toggle
class to the selector and existing password fields.