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 navigator #73

Merged
merged 9 commits into from
Dec 7, 2020
Merged

Add navigator #73

merged 9 commits into from
Dec 7, 2020

Conversation

jasalisbury
Copy link

@jasalisbury jasalisbury commented Nov 20, 2020

Description of change

The navigator allows us to split up large forms into multiple pages. It has a side navigation that allows any page to be selected. The side navigation displays the state of each page and is stickied to the top of the page. Some notes:

  • Adds a new library react-helmet to help update page titles. This library helps keep the page title updated with the page the user is viewing in the activity report.
  • Reduced the use of act(...) in tests. Previously we would get warnings that looked like Warning: An update to *COMPONENT* inside a test was not wrapped in act(...).. This indicates updates to the DOM were happening outside of a test. I've updated tests to not need to be wrapped in act. Some tests still use act but they deal with third party libraries or using mock timers.
  • Started replacing fireEvent with userEvent per this suggestion. Will make a separate PR to switch everything over
  • The activity summary page is pretty long, it would not surprise me if it gets broken down into multiple pages at some point.
  • After Fix url routing #62 is merged in I'll update this branch to deploy to sandbox (unless you are using sandbox @SarahJaine!).

How to test

  1. Browse to https://tta-smarthub-sandbox.app.cloud.gov/activity-reports and fill out the form

OR

  1. Pull down branch
  2. Install deps
  3. Start dev environment, login and browse to localhost:3000/activity-report
  4. Fill out the form

Issue(s)

Checklist

  • Meets issue criteria
  • Code tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • [n/a] Documentation updated

The navigator allows us to split up large forms into multiple pages. It
has a side navigation that allows any page to be selected. The side nav
displays the state of each page.
Switch from using fireEvent to userEvent. Will update additional tests
as I come accross them. Reduce the instances of using "act" in tests,
now we properly wait for all the rerenders to occur. Some tests still
need to use act. Most deal with third party libraries
Copy link
Collaborator

@kryswisnaskas kryswisnaskas left a comment

Choose a reason for hiding this comment

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

Very nice!

A lot of great features.

I did notice the UI jump a bit when navigating sections, but this is minor.

I wonder if we should start adding validations to the UI. I know we have a separate PR for validations. Do you think it would be easier to handle them all at once or start implementing them at least partially, e.g. the file uploads validations?

We should probably also add some cucumber tests, either here or in a different PR.

frontend/src/App.js Show resolved Hide resolved
frontend/src/components/Navigator/components/SideNav.js Outdated Show resolved Hide resolved
frontend/src/components/Navigator/index.js Show resolved Hide resolved
@jasalisbury
Copy link
Author

I wonder if we should start adding validations to the UI. I know we have a separate PR for validations. Do you think it would be easier to handle them all at once or start implementing them at least partially, e.g. the file uploads validations?

There shouldn't be too many fields that require UI validations as most fields are select-boxes or radio/check-boxes. The fields that do require validations (text or number fields) should be easy to use because we are using react-hook-form. I would suggest we add the missing validations in one PR and whenever we add additional fields to the form in the future we add the validation as well.

We should probably also add some cucumber tests, either here or in a different PR.

My vote would be a different PR, this PR is already pretty big.

@kryswisnaskas kryswisnaskas self-requested a review December 7, 2020 20:44
@jasalisbury jasalisbury merged commit e320740 into main Dec 7, 2020
@jasalisbury jasalisbury deleted the js-140-add-navigator branch December 7, 2020 20:52
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.

2 participants