-
Notifications
You must be signed in to change notification settings - Fork 13
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
Nj 182 review screen #5303
base: main
Are you sure you want to change the base?
Nj 182 review screen #5303
Conversation
Heroku app: https://gyr-review-app-5303-776a7dcd6501.herokuapp.com/ |
<h3 class="text--body spacing-below-5"><%=t(".household_rent_own") %></h3> | ||
<p><%= current_intake.household_rent_own %></p> | ||
<h3 class="text--body spacing-below-5"><%=t(".household_rent_own", filing_year: current_tax_year) %></h3> | ||
<p><%=t(".household_rent_own_#{current_intake.household_rent_own}") unless current_intake.household_rent_own_unfilled? %></p> |
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.
[pebble] Should the unless be an if statement starting above the h3 instead? Do we want that header hanging out with an empty p tag if the field is unfilled?
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.
This unless
should never actually be needed in practice but was coming up in a test case review_page_spec.rb
that was failing because it's testing the review screen without filling in all mock data.
In real life, it's not possible for a user to get to this page if the question is unfilled. But because of the interpolation, if it is unfilled in test data, you get a No Translation Key error. So this prevents that error but should never impact the user experience.
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.
doesn't seem like a blocker, but that does seem like something to fix in the test, and not have unwanted code in the implementation for test case setup
</section> | ||
<% end %> | ||
|
||
<% if current_intake.filing_status_qw? %> |
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.
[dust] - Is there a guideline about when we use qw
vs qss
?
<% unless current_intake.spouse_disabled_unfilled? %> | ||
<h2 class="text--body text--bold spacing-below-5"><%= t(".spouse_disabled") %></h2> | ||
<p><%= current_intake.spouse_disabled_yes? ? t("general.affirmative") : t("general.negative") %></p> | ||
<% 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.
[pebble] there seem to be a few of these <% end %>
with two spaces throughout the file, some already here and some new
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.
no blockers from me!
Link to pivotal/JIRA issue
https://github.com/newjersey/affordability-pm/issues/182
Is PM acceptance required? (delete one)
What was done?
<section>
elementsHow to test?
qss
profileScreenshots (for visual changes)