-
Notifications
You must be signed in to change notification settings - Fork 166
Update State ID hint, move HTML out of translation strings into template #9617
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
Changes from all commits
9fdb07d
c01cd61
1711e52
b0b839e
4aa077c
8e71e5c
2f24d13
2473e90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,10 +111,37 @@ | |
| ) %> | ||
| </div> | ||
| <div class="margin-bottom-5"> | ||
| <% state_id_number_hint_default = capture do %> | ||
svalexander marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <%= t('in_person_proofing.form.state_id.state_id_number_hint') %> | ||
| <% [ | ||
| [t('in_person_proofing.form.state_id.state_id_number_hint_spaces'), ' '], | ||
| [t('in_person_proofing.form.state_id.state_id_number_hint_forward_slashes'), '/'], | ||
| [t('in_person_proofing.form.state_id.state_id_number_hint_asterisks'), '*'], | ||
| [t('in_person_proofing.form.state_id.state_id_number_hint_dashes'), '-', true], | ||
| ].each do |text, symbol, last| %> | ||
| <span class="usa-sr-only"><%= text %><%= ',' if !last %></span> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the comma was probably the trickiest bit! I think leaving the comma inside each string is error prone. if we were joining "plain" text, I would consider having us use someting like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nice solution :superb: (needs a emoji)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very easy to grok. I say leave it. Seems like Array#to_sentence would require you to pass in the wrapping HTML for the conjoining elements as a param? This is just much more explicit. |
||
| <span aria-hidden="true"><%= symbol %></span> | ||
| <% end %> | ||
| <% end %> | ||
|
|
||
| <% state_id_number_hint = capture do %> | ||
| <% [ | ||
| [:default, state_id_number_hint_default], | ||
| ['TX', t('in_person_proofing.form.state_id.state_id_number_texas_hint')], | ||
| ].each do |state, hint| %> | ||
| <%= content_tag( | ||
| :span, | ||
| hint, | ||
| class: state == :default ? nil : 'display-none', | ||
| data: { state: }, | ||
| ) %> | ||
| <% end %> | ||
| <% end %> | ||
|
|
||
| <%= render ValidatedFieldComponent.new( | ||
| name: :state_id_number, | ||
| form: f, | ||
| hint: t('in_person_proofing.form.state_id.state_id_number_hint'), | ||
| hint: state_id_number_hint, | ||
| hint_html: { class: ['tablet:grid-col-10', 'jurisdiction-extras'] }, | ||
| input_html: { value: pii[:state_id_number] }, | ||
| label: t('in_person_proofing.form.state_id.state_id_number'), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe 'idv/in_person/state_id.html.erb' do | ||
| let(:pii) { {} } | ||
| let(:form) { Idv::StateIdForm.new(pii) } | ||
| let(:parsed_dob) { Date.new(1970, 1, 1) } | ||
|
|
||
| before do | ||
| allow(view).to receive(:url_for).and_return('https://example.com/') | ||
| end | ||
|
|
||
| subject(:render_template) do | ||
| render template: 'idv/in_person/state_id', | ||
| locals: { updating_state_id: true, form: form, pii: pii, parsed_dob: parsed_dob } | ||
| end | ||
|
|
||
| it 'renders state ID hint text with correct screenreader tags', aggregate_failures: true do | ||
| render_template | ||
|
|
||
| doc = Nokogiri::HTML(rendered) | ||
|
|
||
| jurisdiction_extras = doc.at_css('.jurisdiction-extras') | ||
|
|
||
| all_hints = jurisdiction_extras.css('[data-state]') | ||
| shown = jurisdiction_extras.css('[data-state]:not(.display-none)') | ||
| hidden = jurisdiction_extras.css('[data-state].display-none') | ||
|
|
||
| expect(shown.size).to eq(1), 'only shows one hint' | ||
| expect(shown.size + hidden.size).to eq(all_hints.size) | ||
|
|
||
| default_hint = jurisdiction_extras.at_css('[data-state=default]') | ||
| default_hint_screenreader_tags = default_hint.css('.usa-sr-only') | ||
| *first, last = default_hint_screenreader_tags.map(&:text) | ||
| expect(first).to all end_with(',') | ||
| expect(last).to_not end_with(',') | ||
|
Comment on lines
+34
to
+35
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specs to make sure we get the commas correct inside the reader hint texts
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
| end | ||
| end | ||
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.
do we need
display-none? it seemsshouldShowwould always be true.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.
We need to show one but hide all the others, so this does that in one loop