Conversation
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, can we also add some before/after screenshots to changes like this next time? Makes it easier to see what's going on
app/views/shared/_ssn_field.html.erb
Outdated
| <%# maxlength set and includes '-' delimiters to work around cleave bug %> | ||
| <%= f.label :ssn do %> | ||
| <strong> | ||
| <%= t('idv.form.ssn_label_html') %> | ||
| </strong> | ||
| <% end %> | ||
| <div class="margin-bottom-1 text-hint"> | ||
| <em><%= t('forms.example') %></em> | ||
| <em>123-45-6789</em> | ||
| </div> | ||
| <%= f.input( |
There was a problem hiding this comment.
| <%# maxlength set and includes '-' delimiters to work around cleave bug %> | |
| <%= f.label :ssn do %> | |
| <strong> | |
| <%= t('idv.form.ssn_label_html') %> | |
| </strong> | |
| <% end %> | |
| <div class="margin-bottom-1 text-hint"> | |
| <em><%= t('forms.example') %></em> | |
| <em>123-45-6789</em> | |
| </div> | |
| <%= f.input( | |
| <%= f.label :ssn do %> | |
| <strong> | |
| <%= t('idv.form.ssn_label_html') %> | |
| </strong> | |
| <% end %> | |
| <div class="margin-bottom-1 text-hint"> | |
| <em><%= t('forms.example') %></em> | |
| <em>123-45-6789</em> | |
| </div> | |
| <%# maxlength set and includes '-' delimiters to work around cleave bug %> | |
| <%= f.input( |
Can we move the comment down closer to the input with maxlength set?
app/views/shared/_ssn_field.html.erb
Outdated
| <%= f.label :ssn do %> | ||
| <strong> | ||
| <%= t('idv.form.ssn_label_html') %> | ||
| </strong> | ||
| <% end %> |
There was a problem hiding this comment.
Could we use the design system usa-label class here, which also adds the bolded effect?
| <%= f.label :ssn do %> | |
| <strong> | |
| <%= t('idv.form.ssn_label_html') %> | |
| </strong> | |
| <% end %> | |
| <%= f.label :ssn, class: 'usa-label' do %> | |
| <%= t('idv.form.ssn_label_html') %> | |
| <% end %> |
app/views/shared/_ssn_field.html.erb
Outdated
| <%= t('idv.form.ssn_label_html') %> | ||
| </strong> | ||
| <% end %> | ||
| <div class="margin-bottom-1 text-hint"> |
There was a problem hiding this comment.
My changes in #5481 are planning to remove this text-hint class. Can we use usa-hint as well?
I'm also a bit curious about the italicized effect, since that's not baked into the usa-hint class as well, though I think it would make sense to do that if that's how we'd want hints to appear.
There was a problem hiding this comment.
The proposed change in Figma calls for the italicized effect, so thats why I reached for the<em>.
There was a problem hiding this comment.
The proposed change in Figma calls for the italicized effect, so thats why I reached for the
\<em\>.
As discussed on Slack, it sounds like we'll want to use italics universally for hints. I can make that change in identity-style-guide, but in the meantime, maybe we add some temporary styles for .usa-hint { font-style: italic; } in the IdP?
There was a problem hiding this comment.
I can make that change in the IdP. I will also go ahead and remove .text-hint from the HTML and CSS as we are using it for the phone number hint.
app/views/shared/_ssn_field.html.erb
Outdated
There was a problem hiding this comment.
Do we need the spans ?
| <span><%= t('forms.example') %></span> | |
| <span>123-45-6789</span> | |
| <%= t('forms.example') %> 123-45-6789 |
There was a problem hiding this comment.
I used the span here to keep both of the elements inline, but after removing them, I realize we don't need them anymore.
0e10156 to
dd6220d
Compare
This PR adds help text for users to enter their social security number
Why: So that users can understand how to enter their social security number
Edited to add screenshots