#2839: JS Refactor - [RJM]

[rachidatecs]

Ticket

Resolves #2839
Resolves #2840

Changes

• IMPORTANT: This PR introduces updates to package.json. You will need to remove node_modules and package-lock.json, then run npm install on your local. <<< @therealslimhsiehdy
• Pinned down uswds and uswds-compiler to exact versions. This was done for better control over USWDS updates.
• Extended gulp with babel and webpack integrations
• Created jobs to watch, transpile and tree-shake JS from modules and modules-admin folders into get-gov.js and get-gov-admin.js
• Source mapping allows debuggers to trace code back to source JS modules
• Deconstructed app get-gov.js into helpers and modules, with a main.js entry point
• Deconstructed DJS get-gov-admin.js into helpers and page-level modules, with a main.js entry point
• NOTE: We could theoretically separate out common helpers into a directory outside modules and modules-admin, but then changes to these files would not trigger the watcher and the transpilation/tree-shaking, so I chose to keep a bit of repetition within modules and modules-admin
• NOTE 2: @zandercymatics - you know I think there's such a thing as too much modularity and abstraction, so I kept related chunks of code in the same files. This is especially pertinent to DJA change-form methods and IIFEs.
• NOTE 3: Edited .gitignore to track the new modular files, deleted the old non-dynamic get-gov.js and get-gov-admin.js and (hopefully) got git to forget about tracking them. <<< @therealslimhsiehdy

Context for reviewers

Due to the size of the refactor, we unfortunately have to review every single JS interaction across the site to make sure it's all still functional. The code refactor did not include any re-writes but everything got moved around then glued back.

We should test changing the old get-gov and get-gov-admin files by hand on local environments other than mine and make sure they do not get tracked in git.

I think the most realistic way to get this reviewed is to go over the JS modules and just test their functionality in the app. Aside from moving files and directories around, I have done very little 'spot' refactoring on the JS. Any edits would have been linting style edits or adding a console.warn on a couple of admin helpers (you'll see it).

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

• Met the acceptance criteria, or will meet them in a subsequent PR
• Created/modified automated tests
• Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

• If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
• Interactions with external systems are wrapped in try/except
• Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

• Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist
• Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
• Checked keyboard navigability
• Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

• Pulled this branch locally and tested it
• Verified code meets all checks above. Address any checks that are not satisfied
• Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
• Checked that all code is adequately covered by tests
• Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

• New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
• Checked keyboard navigability
• Meets all designs and user flows provided by design/product
• Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
• (Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

• Checked that the design translated visually
• Checked behavior. Comment any found issues or broken flows.
• Checked different states (empty, one, some, error)
• Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

• Checked keyboard navigability
• Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
• Tested with multiple browsers (check off which ones were used)
  • Chrome
  • Microsoft Edge
  • FireFox
  • Safari
• (Rarely needed) Tested as both an analyst and applicant user

References

• Code review best practices

Screenshots

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[dave-kennedy-ecs]

[non blocking comment] The directory structure does not make sense to me. Specifically, assets/modules and assets/modules-admin. I looked for some standard practices, and suggest that instead we consider using assets/src/js for the source js files. I don't know what is the best structure under that. Maybe even getgov and getgov-admin subdirectories. Also, should the generated files be named getgov.min.js and getgov-admin.min.js?

[zandercymatics]

you know I think there's such a thing as too much modularity and abstraction, so I kept related chunks of code in the same files. This is especially pertinent to DJA change-form methods and IIFEs.

Lol, understandable and sick burn

[zandercymatics]

initial review of some files, not everything

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[allly-b]

When I try to remove a member I get a bad request (This is on the members table, click the kebab, then click remove member)
Error message:

[allly-b]

clicking "manage" on a member in the members table results in this showing in console log:

[rachidatecs]

When I try to remove a member I get a bad request (This is on the members table, click the kebab, then click remove member) Error message:

I believe this is normal and desirable when the remove operation gets blocked for a valid reason (ie, the member has active requests). See if you're getting an error alert on the page when that happens.

[rachidatecs]

clicking "manage" on a member in the members table results in this showing in console log:

Fixed

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[allly-b]

Other employees on the request form has a bug now clicking the radio isn't toggling to only show the explanation box and instead still shows "other employees"

[allly-b]

Rachid you were right that there was an error at the top of the screen. We should be scrolling to top & giving the error focus, but it looks like that is non on development either, seems like a separate issue we need to address in a different ticket.

I see Zander has comments for additional comments in the code, feel free to just make a ticket for adding explanations since I know the longer this sits the more js issues you have to work in. Besides the one bug mentioned above, everything looks good. Happy to approve once the bug is fixed.

[rachidatecs]

Rachid you were right that there was an error at the top of the screen. We should be scrolling to top & giving the error focus, but it looks like that is non on development either, seems like a separate issue we need to address in a different ticket.

I see Zander has comments for additional comments in the code, feel free to just make a ticket for adding explanations since I know the longer this sits the more js issues you have to work in. Besides the one bug mentioned above, everything looks good. Happy to approve once the bug is fixed.

I'll add some comments in gulpfile shortly.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.

[zandercymatics]

Awesome comments here and on createBundleTask

[allly-b]

I think we are good to merge this. There may be something that slipped through but I think we can all just be mindful to watch for any minor glitches. All major functionality has been checked.

[zandercymatics]

Most of everything looks good. I'd recommend Alysia taking a glance too, if I missed something

[github-actions]

🥳 Successfully deployed to developer sandbox rjm.