USWDS - Modal: JS Refactor by mahoneycm · Pull Request #5315 · uswds/uswds

mahoneycm · 2023-06-01T19:28:12Z

Summary

Refactored Modal JavaScript for added consistency and failsafe measures. This resolves issues when initialization is called more than once on a document.

Breaking change

This is not a breaking change.

Related issue

Closes #5062

Related pull requests

Changelog PR →

Continued from #5208

Preview link

Preview link:
Modal →

Problem statement

From #5062:

In JS frameworks (React, Vue, Angular) when multiple instances of a dynamic uswds components are dynamically created (modals, datepickers...), calling init/on in the create lifecycle and off in the destroy lifecycle is often buggy and results in multiple inits or off() on components that are already torn down.

After inspecting Modal JS, we saw room for improvement to align JS to be more consistent with other Dynamic Components, including adding error messages if key pieces are missing from component code.

Solution

Refactor Modal JS to:

Be more consistent with other dynamic USWDS components
Break out large setUpModal() function into more palatable, feature-specific functions
Add error's when component code is missing key pieces such as id, aria-labelledby, and aria-describedby.

Testing and review

Review

Code Refactor:

Visit modal component page
Confirm no visual or functional regression
Review updated JS
- Confirm code follows standard dynamic component patterns
- Check for possible improvements regarding organization and variable names.
On your local, remove ID, aria-labelledby, and/or aria-describedby from usa-modal, allow the preview to rebuild, then inspect the component preview.
Confirm error message in console.

Multiple init in JS Frameworks

Checkout this PR and run npm link
Clone this test repo
Run npm install npm run start
Confirm error on the multiple models page
Return to code and run npm link @uswds/uswds
Return to app and confirm modals work correctly.
The "two modals" page will have a console error because init is called a second time (once per modal)
- The second init searches for usa-modal class, which has been moved to hidden modal window element
- This element does not have an ID, which is necessary for component code to dynamically create modal component
- Initialization is then stopped, preventing the error we saw before.
- Ideally, the user would not call initialization more than once on each modal, which can be don't by setting the target root

Testing

No visual or functional regressions to modal.
Component uses behavior pattern.
Removing ID, aria-labelledby, and aria-describedby from usa-modal triggers error in console.
Resolves multiple init bug caused when initiating multiple Modals using a JS framework (Follow test repo instructions above).

Before opening this PR, make sure you’ve done whichever of these applies to you:

Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
Run npm test and confirm that all tests pass.

…dal-behavior

mahoneycm · 2023-07-03T20:04:18Z

@mejiaj @amyleadem Bumping for review when you have the bandwidth!

mejiaj

Good work here! I've added additional comments & suggestions below.

What I tested in sequential order:

PR description: Is PR description accurate and clear. Added suggestion below.
No Regressions: Have there been any regressions in visuals/functionality.
New features: Removing ID, aria-labelledby, and aria-describedby from usa-modal triggers error in console.
Code: Does it meet USWDS code standards for formatting and code style.

PR description

The items I tested related to process. Added suggestions below.

What I tested:

Changelog PR exists & is linked.
Preview link exists & is accurate.
Testing & Review is clear & reflect changes.
- ⚠️ This is a complex PR, so we could improve clarity by adding a new list that has expectations.

Suggestions for Testing & Review section:

### Review

1. Steps.
2. To.
3. Review.

### Testing

- [ ] No visual or functional regressions to modal.
- [ ] Component uses behavior pattern.
- [ ] Removing `ID, aria-labelledby, and aria-describedby` from `usa-modal` triggers error in console.
- [ ] Repo instructions ― I haven't gone through these steps.

Note
This would just be a new section that only highlights expectations for this work to be considered "done."

Code

In general, please be sure to add JSDoc comments to functions.

packages/usa-modal/src/index.js

mejiaj · 2023-07-05T17:45:51Z

packages/usa-modal/src/index.js

-  ) {
-    const attribute = modalContent.attributes[attributeIndex];
+
+  modalAttributes.forEach((attribute) => {


Nice improvement!

mejiaj · 2023-07-05T19:40:11Z

packages/usa-modal/src/index.js

-  modalWrapper.appendChild(modalContent);
-  modalContent.parentNode.insertBefore(overlayDiv, modalContent);
-  overlayDiv.appendChild(modalContent);
+const setModalAttributes = (baseComponent, targetWrapper) => {


Can we add a JSDoc comment to this and any new functions?

Does targetWrapper mean the modal content wrapper?

Resolved in 0e34893

and yes! targetWrapper is the wrapper made for the individual modal component on init

Would it make sense to rename targetWrapper to something more descriptive like modalContentWrapper?

I think this adds clarity! I'll add it

packages/usa-modal/src/index.js

mejiaj · 2023-07-06T13:11:26Z

packages/usa-modal/src/index.js

+  modalContent.parentNode.insertBefore(modalWrapper, modalContent);
+  modalWrapper.appendChild(modalContent);
+  modalContent.parentNode.insertBefore(overlayDiv, modalContent);
+  overlayDiv.appendChild(modalContent);


Is there any way to simplify these (lines 247-50) to make it easier to track what's being appended? Here's what I observed when testing:

Go to usa-modal parent, in this component library it's div.margin-y-3.

Empty div is created and immediately inserted.

Another empty div is created inside as a sibling to usa-modal.

Finally we've appended empty divs to DOM and modifying their classes ― ⚠️ this doesn't follow the usual pattern of creating DOM and necessary attributes in JS.

It would make more sense to:

Create elements.

Add attributes.

Ensure structure is what we want.

Append fully scaffolded HTML to DOM.

In this specific example, we need the parent overlay div on the wrapper so that setModalAttributes() can properly set up the modal's closers.

So the process looks a little more like:

Create elements

Add classes

Structure overlay, wrapper, and content in correct order

Set attributes

Append fully scaffolded HTML to DOM

Take a look at this updated code and let me know what you think!

Updated in 270c293

packages/usa-modal/src/index.js

mejiaj

Nice work. I've tested the following:

No visual regressions.
No functional regressions.
No a11y regressions.
New features: Errors when missing id, aria-labelledby, aria-describedby.
No issues with multiple modals; tested two modals on same page.

mahoneycm · 2023-07-25T14:09:15Z

@amyleadem bumping for review when you have time!

amyleadem

This is looking good! Had just a few notes about opportunities to improve the comments for quicker understanding.

I performed the following checks:

Confirm that multiple modals are on a page works in the test repo
Confirm no visual regressions in Storybook
Confirm no functional regressions in Storybook
Confirm that removing id, aria-labelledby, or aria-describedby attributes causes console error
Confirm that code style and comments meet standards
- Added some comments about adding detail to the function comments
Confirm changelog
- Added some comments to the changelog PR

packages/usa-modal/src/index.js

amyleadem · 2023-07-25T17:27:07Z

packages/usa-modal/src/index.js

-  modalWrapper.appendChild(modalContent);
-  modalContent.parentNode.insertBefore(overlayDiv, modalContent);
-  overlayDiv.appendChild(modalContent);
+const setModalAttributes = (baseComponent, targetWrapper) => {


Would it make sense to rename targetWrapper to something more descriptive like modalContentWrapper?

packages/usa-modal/src/index.js

…ntain consistency

amyleadem

Thanks for adding those clarifying comments, @mahoneycm. Looks good to me!

I performed the following checks:

Confirm that multiple modals are on a page works in the test repo
Confirm no visual regressions in Storybook
Confirm no functional regressions in Storybook
Confirm that removing id, aria-labelledby, or aria-describedby attributes causes console error
Confirm that code style and comments meet standards
Confirm changelog

mejiaj · 2023-08-02T17:00:39Z

packages/usa-modal/src/index.js

-  const modalContent = baseComponent;
-  const modalWrapper = document.createElement("div");
-  const overlayDiv = document.createElement("div");
+const createPlaceHolder = (baseComponent) => {


Explore cloneNode() [MDN] to allow users to add their own event listeners to modal content. Created a new issue for this #5419.

mejiaj

@mahoneycm can you resolve conflicts, otherwise LGTM?

mahoneycm · 2023-10-16T15:56:58Z

@mejiaj conflicts resolved! 👍

Amy's work in #5493 is included and working as expected

Changes complete

mahoneycm and others added 15 commits March 27, 2023 12:36

Utilize behaviors on modal

eeed2c1

Add modalId conditional statement before running setUpModal

11fc665

Refine modal behaviors & conditional statement

b634bd1

Run prettier formatting

1f972ce

Comment adjustments

ee866c9

Replace selectAll with selectOrMatches

1c51556

Merge branch 'cm-modal-behavior' of github.com:uswds/uswds into cm-mo…

4e13fc0

…dal-behavior

Update modalTrigger anchor comments

24a90b7

Move modalTrigger selector from root to document to resolve unit test

0988c22

Modify conditional statement and add check to teardown

76b3f21

Refactor setUpModal into multiple functions

a37296c

Refactor placeholder attribute loop

fcc6aff

Add error messages and refactor cleanUpModal attribute loop to match

d74192c

Prettier:js

417b41f

Merge branch 'develop' of github.com:uswds/uswds into cm-refactor-modal

fc4e134

mahoneycm mentioned this pull request Jun 1, 2023

Create modal changelog item uswds/uswds-site#2117

Merged

mahoneycm requested review from amyleadem and mejiaj June 2, 2023 13:27

mahoneycm marked this pull request as ready for review June 2, 2023 13:27

mahoneycm mentioned this pull request Jun 2, 2023

USWDS - Modal: Implement behaviors and add conditional initialization #5208

Closed

3 tasks

Merge branch 'develop' of github.com:uswds/uswds into cm-refactor-modal

aceec33

mejiaj mentioned this pull request Jul 6, 2023

USWDS - Bug: Modal removes existing padding on close #5361

Closed

mejiaj suggested changes Jul 6, 2023

View reviewed changes

mejiaj reviewed Jul 6, 2023

View reviewed changes

packages/usa-modal/src/index.js Outdated Show resolved Hide resolved

mahoneycm added 5 commits July 10, 2023 12:20

Remove redundant aria label checks

cdd38a0

Remove comment

a5879b4

Update function comments

0e34893

Simplify rebuildModal to be more readable

270c293

Run prettier && lint

d5225f8

Remove log

01b190e

mahoneycm requested a review from mejiaj July 11, 2023 18:16

mahoneycm added 2 commits July 11, 2023 14:55

JSDoc comment cleanup

660bb58

Run prettier && lint

9e57309

mejiaj approved these changes Jul 14, 2023

View reviewed changes

amyleadem suggested changes Jul 25, 2023

View reviewed changes

mahoneycm added 2 commits July 25, 2023 14:54

Update JSDoc comments per recommendations

c84d5ed

Swap remaining modalWrapper const names to modalContentWrapper to mai…

e519bf1

…ntain consistency

mahoneycm requested a review from amyleadem July 26, 2023 20:02

amyleadem approved these changes Jul 28, 2023

View reviewed changes

amyleadem requested a review from thisisdano July 28, 2023 18:47

mejiaj reviewed Aug 2, 2023

View reviewed changes

mejiaj mentioned this pull request Aug 2, 2023

USWDS - Bug: Modal ignores user event listeners #5419

Closed

Merge branch 'develop' of github.com:uswds/uswds into cm-refactor-modal

3989aa0

This was referenced Sep 13, 2023

USWDS - Modal: Remove padding style rules after modal close #5493

Merged

Query the DOM for .usa-modal-wrapper before building or destroying mo… #5031

Closed

mejiaj previously requested changes Oct 11, 2023

View reviewed changes

Merge branch 'develop' of github.com:uswds/uswds into cm-refactor-modal

c45e695

mahoneycm requested a review from mejiaj October 20, 2023 19:33

amyleadem mentioned this pull request Oct 27, 2023

Draft release notes for 3.7.0 #5585

Closed

thisisdano approved these changes Oct 30, 2023

View reviewed changes

thisisdano merged commit 2eec2cd into develop Oct 30, 2023

thisisdano deleted the cm-refactor-modal branch October 30, 2023 23:16

amyleadem mentioned this pull request Nov 6, 2023

USWDS-Site: Documentation updates for release 3.7.0 uswds/uswds-site#2332

Merged

thisisdano mentioned this pull request Nov 9, 2023

USWDS 3.7.0 #5605

Merged

werdnanoslen mentioned this pull request Dec 27, 2023

feat: updates for USWDS v3.7 trussworks/react-uswds#2680

Merged

7 tasks

sennhann29 mentioned this pull request Jul 26, 2024

[Snyk] Upgrade @uswds/uswds from 3.4.1 to 3.8.1 sennhann29/digitalgov.gov#4

Open

Conversation

mahoneycm commented Jun 1, 2023 • edited by amyleadem Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking change

Related issue

Related pull requests

Preview link

Problem statement

Solution

Testing and review

Review

Code Refactor:

Multiple init in JS Frameworks

Testing

Uh oh!

mahoneycm commented Jul 3, 2023

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

What I tested in sequential order:

PR description

Code

Uh oh!

Uh oh!

Uh oh!

mejiaj Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

mejiaj Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

mahoneycm Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

amyleadem Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mejiaj Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

mahoneycm commented Jul 25, 2023

Uh oh!

amyleadem left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amyleadem Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amyleadem left a comment

Choose a reason for hiding this comment

Uh oh!

mejiaj Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

mahoneycm commented Jun 1, 2023 •

edited by amyleadem

Loading

amyleadem Jul 25, 2023 •

edited

Loading

mejiaj Jul 6, 2023 •

edited

Loading

amyleadem left a comment •

edited

Loading

amyleadem Jul 25, 2023 •

edited

Loading

mejiaj Aug 2, 2023 •

edited

Loading