USWDS - Modal: Implement behaviors and add conditional initialization#5208
USWDS - Modal: Implement behaviors and add conditional initialization#5208mahoneycm wants to merge 10 commits into
Conversation
mejiaj
left a comment
There was a problem hiding this comment.
Thanks for updating this component. Functionality looks good. Added a few comments to potentially improve clarification. Could you also update the PR description? Not seeing links to preview or the test repo mentioned.
I've tested:
- Code style (I ran prettier)
- Run
npm run test - Functionality in StorybookJS
mejiaj
left a comment
There was a problem hiding this comment.
Thanks for taking this on! Added a comment and a question.
|
@rachidatecs are you able to help us test? I also noticed that the initialization runs twice in your test repo. So if I have two modals, it'll run |
|
Happy to pull your branch and test the new code. As to your question on init running twice in the test repo - that may well be the point of the branch you're on, to demo the issue. Which branch are you running? |
|
@rachidatecs thank you! For confirming the issue, I was on It makes sense why the first two modals have values and the other two just have |
|
@mejiaj That's a result of the second initialization. When it's called a second time, it selects the rebuilt The null values are because the new |
|
Yep, that's actually the issue we're trying to solve. We can skirt around it but requiring the package at the page level (the app level will cause modals not on the landing page to not init, and the module level will cause those duplicate inits when more that one modal is present on a page) but that's really a workaround that avoids the issue. In other words: when the init is called on the component level in a modern js framework (tested in angular and vue, assume it's the same in react), the init method fires every time a modal placeholder is detected in the markup (one init for each modal component on the page). So say you have 3 modals, the first init from the first component will cause all 3 modals to init. The second component will cause all 3 modals to init again. The 3rd component will cause all 3 modals to init a third time. You would end up with 9 generated modals, 3 that are integral and 6 that are missing ids. |
|
Closing in favor of #5315 |
Summary
Implements behaviors utility and queries DOM before initialization. Now
modalfollows the pattern set by other dynamically created components by using the behaviors utility. Additionally, the component will query the DOM before building the component to prevent errors caused by multiple initializations being called. This is beneficial for component-level initialization.Breaking change
This is not a breaking change.
Related issue
Closes #5062
Preview link
Modal (federalist preview)
Test repo (Follow instructions below)
Problem statement
When using modern JS frameworks such as Angular or React, calling on the component to initialize multiple times caused the modal's functionality to break. The error occurred because the initialization process selects all
usa-modaldivs and rebuilds them using their ID. The newly generated div removes the ID and moves it to the wrapper.Once initialization is called again, it selects the new
usa-modaldiv without the ID, which causes build errors due to the ID beingnullSolution
modalIdbefore runningsetUpModalfunctionTesting and review
npm linknpm installnpm run startnpm link @uswds/uswdsNotes
Output HTML before change:
After:
Before opening this PR, make sure you’ve done whichever of these applies to you:
git pull origin [base branch]to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch isdevelop).npm testand confirm that all tests pass.