-
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 293 - move retirement screen to income section #5614
base: main
Are you sure you want to change the base?
Conversation
Heroku app: https://gyr-review-app-5614-b3d138cb6680.herokuapp.com/ |
9c2fe59
to
aa106d3
Compare
Navigation::NavigationSection.new("state_file.navigation.nj.section_5", [ | ||
Navigation::NavigationStep.new(StateFile::Questions::PrimaryStateIdController), # Footer | ||
Navigation::NavigationStep.new(StateFile::Questions::SpouseStateIdController), # Footer | ||
Navigation::NavigationStep.new(StateFile::Questions::NjReviewController), # review controller should come before taxes owed / refund due screens | ||
Navigation::NavigationStep.new(StateFile::Questions::NjReviewController), # REVIEW: controller should come before taxes owed / refund due screens |
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 change to this comment seems like it changes the meaning here in a way I wouldn't expect - makes it sound like a TODO command instead of a note. Is this intentional?
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.
tbh is this comment even necessary at all, even in its original form
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, I think it should be removed. This was an auto-rubocop fix.
config/locales/en.yml
Outdated
@@ -3681,7 +3681,7 @@ en: | |||
retirement_income_source_label: 'Source:' | |||
retirement_income_source_military_pension: U.S. military pension | |||
retirement_income_source_military_survivor_benefit: U.S. military survivor's benefits | |||
retirement_income_source_other: Neither U.S. military pension nor U.S. military survivor's benefits | |||
retirement_income_source_none: Neither U.S. military pension nor U.S. military survivor's benefits |
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] This looks like a merge issue. This should be "other" not "none" given Matt's refactor. Same in the es.yml foile
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.
Thanks for the callout! Will fix.
@@ -27,11 +27,12 @@ def self.show_progress?(controller_class) | |||
# Federal info does not show to users | |||
Navigation::NavigationStep.new(StateFile::Questions::FederalInfoController), | |||
Navigation::NavigationStep.new(StateFile::Questions::DataTransferOffboardingController, false), | |||
], false), | |||
], false), |
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] spacing added in this file? should be removed, or is it rubocop?
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.
Rubocop auto indent
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
How to test?
Screenshots (for visual changes)