Skip to content
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.

Sign up for GitHub

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

Nj 243 - Add headers to review screen #5563

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Nj 243 - Add headers to review screen #5563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc9ce4d
NJ 243 - Add headers to review screen
jachan Feb 11, 2025
c6598e2
updates to heading structure
jachan Feb 11, 2025
9575b6f
fix closing html tags
jachan Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions app/views/state_file/questions/nj_review/edit.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<% if current_intake.dependents.count(&:under_22?).positive? %>
<section id="college-dependents" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".college_dependents") %></h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

[a mound of pebbles] - looks like the h2 -> h3 change was only on the opening tag and not on the closing one. And that seems to apply to all the changes in this PR. it might still render correctly in browsers and test drivers which would hide the issue, but we should fix it

<h3 class="text--body text--bold spacing-below-5"><%=t(".college_dependents") %></h3>
<% current_intake.dependents.each do | dependent | %>
<% if dependent.nj_qualifies_for_college_exemption? %>
<p><%=dependent.full_name %></p>
Expand All @@ -26,7 +26,7 @@
<% if current_intake.dependents.any? && current_intake.has_health_insurance_requirement_exception? %>
<section id="missing-health-insurance" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".missing_health_insurance") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".missing_health_insurance") %></h3>
<% current_intake.dependents.each do | dependent | %>
<% if dependent.nj_did_not_have_health_insurance_yes? %>
<p><%=dependent.full_name %></p>
Expand All @@ -46,7 +46,7 @@
<% if current_intake.filing_status_qw? %>
<section id="year-of-death" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".year_of_death") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".year_of_death") %></h3>
<p><%=current_intake.spouse_death_year %></p>

<%= link_to StateFile::Questions::NjYearOfDeathController.to_path_helper(return_to_review: "y"), class: "button--small" do %>
Expand All @@ -59,7 +59,7 @@

<section id="county-municipality" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".county_municipality", filing_year: current_tax_year) %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".county_municipality", filing_year: current_tax_year) %></h3>
<p><%= t(".county", county: current_intake.county) %></p>
<p><%=current_intake.municipality_name %></p>

Expand All @@ -74,11 +74,11 @@
<section id="disabled_exemption" class="white-group">
<div class="spacing-below-5">
<% unless current_intake.primary_disabled_unfilled? %>
<h2 class="text--body text--bold spacing-below-5"><%= t(".primary_disabled") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%= t(".primary_disabled") %></h3>
<p><%= current_intake.primary_disabled_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% end %>
<% unless current_intake.spouse_disabled_unfilled? %>
<h2 class="text--body text--bold spacing-below-5"><%= t(".spouse_disabled") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%= t(".spouse_disabled") %></h3>
<p><%= current_intake.spouse_disabled_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% end %>
<%= link_to StateFile::Questions::NjDisabledExemptionController.to_path_helper(return_to_review: "y"), class: "button--small" do %>
Expand All @@ -93,11 +93,11 @@
<section id="veterans_exemption" class="white-group">
<div class="spacing-below-5">
<% unless current_intake.primary_veteran_unfilled? %>
<h2 class="text--body text--bold spacing-below-5"><%= t(".primary_veteran") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%= t(".primary_veteran") %></h3>
<p><%= current_intake.primary_veteran_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% end %>
<% unless current_intake.spouse_veteran_unfilled? %>
<h2 class="text--body text--bold spacing-below-5"><%= t(".spouse_veteran") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%= t(".spouse_veteran") %></h3>
<p><%= current_intake.spouse_veteran_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% end %>
<%= link_to StateFile::Questions::NjVeteransExemptionController.to_path_helper(return_to_review: "y"), class: "button--small" do %>
Expand All @@ -111,7 +111,7 @@
<% if current_intake.state_file1099_rs.length.positive? && Flipper.enabled?(:show_retirement_ui) %>
<section id="retirement-income-source" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-25"><%=t(".retirement_income_source") %></h2>
<h3 class="text--body text--bold spacing-below-25"><%=t(".retirement_income_source") %></h3>

<% current_intake.state_file1099_rs.each do |state_file1099_r| %>
<% unless state_file1099_r.state_specific_followup.nil? %>
Expand Down Expand Up @@ -142,9 +142,13 @@
</section>
<% end %>

<div class="spacing-below-25">
<h2 class="text--body text--bold spacing-below-5"><%=t("state_file.navigation.nj.section_3") %></h2>
</div>

<section id="medical_expenses" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".medical_expenses") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".medical_expenses") %></h3>
<p><%= number_to_currency(current_intake.medical_expenses, precision: 0) %></p>
<%= link_to StateFile::Questions::NjMedicalExpensesController.to_path_helper(return_to_review: "y"), class: "button--small" do %>
<%= t(".review_and_edit") %>
Expand All @@ -155,18 +159,18 @@

<section id="property-tax" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".property_tax_credit_deduction") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".property_tax_credit_deduction") %></h3>

<h3 class="text--body spacing-below-5"><%=t(".household_rent_own", filing_year: current_tax_year) %></h3>
<h4 class="text--body spacing-below-5"><%=t(".household_rent_own", filing_year: current_tax_year) %></h4>
<p><%=t(".household_rent_own_#{current_intake.household_rent_own}") unless current_intake.household_rent_own_unfilled? %></p>

<% unless current_intake.property_tax_paid.nil? %>
<h3 class="text--body text--bold spacing-below-5"><%= t(".property_tax_paid", filing_year: current_tax_year) %></h3>
<h4 class="text--body text--bold spacing-below-5"><%= t(".property_tax_paid", filing_year: current_tax_year) %></h4>
<p><%= number_to_currency(current_intake.property_tax_paid, precision: 0) %></p>
<% end %>

<% unless current_intake.rent_paid.nil? %>
<h3 class="text--body text--bold spacing-below-5"><%=t(".rent_paid", filing_year: current_tax_year) %></h3>
<h4 class="text--body text--bold spacing-below-5"><%=t(".rent_paid", filing_year: current_tax_year) %></h4>
<p><%= number_to_currency(current_intake.rent_paid, precision: 0) %></p>
<% end %>

Expand All @@ -180,14 +184,14 @@
<% unless current_intake.claimed_as_eitc_qualifying_child_unfilled? && current_intake.spouse_claimed_as_eitc_qualifying_child_unfilled? %>
<section id="eitc_qualifying_child" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".eitc") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".eitc") %></h3>

<% unless current_intake.claimed_as_eitc_qualifying_child_unfilled? %>
<h3 class="text--body text--bold spacing-below-5"><%=t(".primary_claimed_as_eitc_qualifying_child") %></h3>
<h4 class="text--body text--bold spacing-below-5"><%=t(".primary_claimed_as_eitc_qualifying_child") %></h4>
<p><%=current_intake.claimed_as_eitc_qualifying_child_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% end %>
<% unless current_intake.spouse_claimed_as_eitc_qualifying_child_unfilled? %>
<h3 class="text--body text--bold spacing-below-5"><%=t(".spouse_claimed_as_eitc_qualifying_child") %></h3>
<h4 class="text--body text--bold spacing-below-5"><%=t(".spouse_claimed_as_eitc_qualifying_child") %></h4>
<p><%=current_intake.spouse_claimed_as_eitc_qualifying_child_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% end %>
<%= link_to StateFile::Questions::NjEitcQualifyingChildController.to_path_helper(return_to_review: "y"), class: "button--small" do %>
Expand All @@ -198,9 +202,13 @@
</section>
<% end %>

<div class="spacing-below-25">
<h2 class="text--body text--bold spacing-below-5"><%=t("state_file.navigation.nj.section_4", filing_year: current_tax_year) %></h2>
</div>

<section id="estimated-tax-payments" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".estimated_tax_payments") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".estimated_tax_payments") %></h3>
<p><%= number_to_currency(current_intake.estimated_tax_payments || 0, precision: 0) %></p>
<%= link_to StateFile::Questions::NjEstimatedTaxPaymentsController.to_path_helper(return_to_review: "y"), class: "button--small" do %>
<%= t(".review_and_edit") %>
Expand All @@ -211,7 +219,7 @@

<section id="use-tax" class="white-group">
<div class="spacing-below-5">
<h2 class="text--body text--bold spacing-below-5"><%=t(".use_tax_applied") %></h2>
<h3 class="text--body text--bold spacing-below-5"><%=t(".use_tax_applied") %></h3>
<p><%=current_intake.untaxed_out_of_state_purchases_yes? ? t("general.affirmative") : t("general.negative") %></p>
<% if current_intake.untaxed_out_of_state_purchases_yes? %>
<p class="text--bold spacing-below-5">
Expand All @@ -227,7 +235,7 @@
</section>

<section class="reveal" id="calculation-details">
<h2 class="text--body text--bold spacing-below-5"><button class="reveal__button"><%= t(".reveal.header") %></button></h2>
<h3 class="text--body text--bold spacing-below-5"><button class="reveal__button"><%= t(".reveal.header") %></button></h3>
<div class="reveal__content">
<% @detailed_return_info.each.with_index do |section, i| %>
<div>
Expand Down
30 changes: 20 additions & 10 deletions spec/features/state_file/nj/complete_intake_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,21 @@ def check_xml_results

expect(page).to be_axe_clean.within "main"

groups = page.all(:css, '.white-group').count
h2s = page.all(:css, 'h2').count
expect(groups).to eq(h2s - 2)
# total should be h2s -1 to account for the h2 reveal button
# there's also an extra h2 in the veteran exemption box
household_details_h3s = 2 + dependents_dob
income_details_h3s = 0
dependents_attending_college_h3s = 1
where_you_lived_as_of_dec_31_h3s = 1
disability_exemption_h3s = 1
veterans_exemption_h3s = 2
medical_expenses_h3s = 1
property_tax_h3s = 1
payments_made_h3s = 1
use_tax_h3s = 1
reveal_h3 = 1
total_expected_h3s = household_details_h3s + income_details_h3s + dependents_attending_college_h3s + where_you_lived_as_of_dec_31_h3s + disability_exemption_h3s + veterans_exemption_h3s + medical_expenses_h3s + property_tax_h3s + payments_made_h3s + use_tax_h3s + reveal_h3
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble ] The purpose of the test before was to make sure every white-group section box began with an h2, for consistent structure. Hence comparing the number of h2s to the number of white group boxes.

Now that we're beginning boxes with h3s instead, I think it makes sense to check that structure instead of just counting elements. Maybe checking that each white group has at least one h3? And that it comes before any other content?

Copy link
Contributor

Choose a reason for hiding this comment

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

^ agree that just checking the totals (white groups = h3s) makes more sense. this looks like it's checking that each section has an h3 but since it just adds them up in the end it's actually not a very specific test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with the previous test is that not every white group on the page starts with a single h3 heading. Examples:

The previous test (white-groups=h2s) was not testing what we wanted to test , since abstract_header had been using h3 headings in its white-group sections and we were previously checking against h2 headings.

https://github.com/codeforamerica/vita-min/blob/c75ad162f29742abd773e98eba6e566608530ea3/app/views/state_file/questions/shared/_abstract_review_header.html.erb

I started trying to modify the math to make it work, but with all the cases above, it seemed much more readable to just assert what I expected for each section. Happy to explore other options if folks have suggestions, but just pulling the # of white groups and trying to modify that to match the number of h3s required a lot of arithmetic that seemed really opaque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just read Melanie's comment more closely and found the suggestion to check for the presence of a header before any other content in the white-group, which would make a lot more sense here rather than just asserting that every white-group has an h3. Exploring that solution now!

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use consistent heading structure, and the visual appearance should reflect the markup. It doesn't make sense to start some white groups with h2s and others with h3s-- all of the content should be broken into sections if any of it is. The design might need to be tweaked if this isn't currently possible


h3s = page.all(:css, 'h3').count
expect(total_expected_h3s).to eq(h3s)

edit_buttons = page.all(:css, '.white-group a')
edit_buttons_count = edit_buttons.count
Expand Down Expand Up @@ -569,7 +579,7 @@ def expect_municipality_question_hidden
expect_municipality_question_exists

# unselect county
within find('#county-question') do
within find_by_id('county-question') do
select I18n.t('general.select_prompt')
end
expect_county_question_exists
Expand All @@ -580,7 +590,7 @@ def expect_municipality_question_hidden
advance_to_start_of_intake("Minimal", expect_income_review: false)

select "Atlantic"
within find('#municipality-question') do
within find_by_id('municipality-question') do
expect(page.all("option").length).to eq(24) # 23 municipalities + 1 "- Select -"
expect(page).to have_text "Absecon City"
expect(page).to have_text "Atlantic City"
Expand All @@ -589,7 +599,7 @@ def expect_municipality_question_hidden
end

select "Mercer"
within find('#municipality-question') do
within find_by_id('municipality-question') do
expect(page.all("option").length).to eq(13) # 12 municipalities + 1 "- Select -"
expect(page).to have_text "East Windsor Township"
expect(page).to have_text "Hopewell Township"
Expand All @@ -602,10 +612,10 @@ def expect_municipality_question_hidden

select "Atlantic"
select "Absecon City"
expect(find("#state_file_nj_county_municipality_form_municipality_code").value).to eq("0101")
expect(find_by_id('state_file_nj_county_municipality_form_municipality_code').value).to eq("0101")

select "Mercer"
expect(find("#state_file_nj_county_municipality_form_municipality_code").value).to eq("")
expect(find_by_id('state_file_nj_county_municipality_form_municipality_code').value).to eq("")
end

end
Expand Down
Loading