-
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
FYST-909 Update W2 UI #4928
FYST-909 Update W2 UI #4928
Conversation
Heroku app: https://gyr-review-app-4928-9c5af28a004c.herokuapp.com/ |
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.
Just some minor comments that don't require re-review
<p class="text--bold spacing-below-5"><%= @w2.employee_name %></p> | ||
<p class="spacing-below-25"><%= @w2.employer_name %></p> | ||
|
||
<% if (box_14_codes = StateFile::StateInformationService.w2_supported_box_14_codes(current_state_code)).present? %> |
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] Could you set @box_14_codes
in the edit section of the w2_controller
rather than assign it here? We may not follow that practice 100%, but it's better to use the controller for setting variables in case the way they get used changes in the template.
<p class="spacing-below-25"><%= @w2.employer_name %></p> | ||
|
||
<% if (box_14_codes = StateFile::StateInformationService.w2_supported_box_14_codes(current_state_code)).present? %> | ||
<p class="spacing-below-5"><%= t(".box14_html") %></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.
[dust] this is incredibly minor, but for consistency it would be nice to have box_14
with the underscore everywhere (makes it easier to do a global search)
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.
ohh actually to be it consistent with the other boxes yamls strings i'll make everything for box_14 be box14
Link to pivotal/JIRA issue
https://codeforamerica.atlassian.net/browse/FYST-909
Is PM acceptance required?
Reminder: merge main into this branch and get green tests before merging to main
What was done?
How to test?
Screenshots (for visual changes)