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.

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

Add additional comments to the summary page #28

Merged
merged 4 commits into from
May 20, 2024

Conversation

tdooner
Copy link
Contributor

@tdooner tdooner commented May 17, 2024

Add additional comments to summary page

Fixes FFS-771.

Solution

  • Bring in UswdsFormBuilder
    This class was written for another Nava project, and can be useful for
    us to build off in order to produce USWDS form styling. In order to get
    the tests running, I had to bring in Capybara, which seems okay since we
    will probably want to use it soon for integration testing anyway.
  • Add comments field to "summary" page
    • Adds a form field to the "summary" page that prompts the user if they
      had any other information that would affect the income figures
    • The data is stored in a new column on CbvFlow model:
      additional_information
    • Move the "Download PDF" and "Send to Caseworker" buttons to a new
      page called "share" (since, when submitting a form, the user will need
      a different page to land on).
    • Unrelatedly, change some i18n strings to use relative paths rather
      than absolute paths.

Result

Screenshot 2024-05-17 at 2 42 16 PM Screenshot 2024-05-17 at 2 42 25 PM

Test Plan

Unit/controller tests included.

This class was written for another Nava project, and can be useful for
us to build off in order to produce USWDS form styling. In order to get
the tests running, I had to bring in Capybara, which seems okay since we
will probably want to use it soon for integration testing anyway.
* Adds a form field to the "summary" page that prompts the user if they
  had any other information that would affect the income figures
* The data is stored in a new column on `CbvFlow` model:
  `additional_information`
* Move the "Download PDF" and "Send to Caseworker" buttons to a new
  page called "share" (since, when submitting a form, the user will need
  a different page to land on).
* Unrelatedly, change some i18n strings to use relative paths rather
  than absolute paths.
Copy link
Contributor

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

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

Just some non-blocking thoughts/ideas. Curious if I need to implement the custom form builder for the employer search form.

Comment on lines +18 to +21
if request.patch?
@cbv_flow.update(summary_update_params)
return redirect_to next_path
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense but soon we might need to start splitting this out into multiple controllers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. This is officially getting to be Too Much™. I've got an idea for it. I'll make a task for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Fabulous... it's too bad Nava doesn't have this published as gem or something. Wonder if we could separate it into a local gem folder? I don't feel too strongly about that, just a thought.

Separately, though, does this need to be applied to the employer search form, too? Would love to use the same FormBuilder. And would like to see that it works with the Hotwire feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gem would be cool, agreed, but not much return on investment unless a second team wants to actively use it. And yeah, probably should use it for the employer search. I just asked in #engineering.

@@ -0,0 +1,14 @@
<h2>
<%= t('.header') %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the relative dot paths

@tdooner tdooner merged commit 86aef84 into main May 20, 2024
7 checks passed
@tdooner tdooner deleted the ffs-771-add-comments-to-summary branch May 20, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants