USWDS - Banner: Add accordion dependency #5242

mejiaj · 2023-04-17T20:02:07Z

Summary

Banner initializes without external dependencies. USA banner module now initializes on it's own without the user having to separately initialize accordion.

Note
Because accordion classes are used within banner we have to initialize accordion from within banner. This means that a user could initialize accordion and expect their banners to work and vice versa.

We don't see this issue when projects import uswds.js because it includes accordion already. Now you should be able to import banner and only banner and have it work as expected.

Breaking change

This is not a breaking change.

Related issue

Closes #5179.

Related pull requests

Need to create changelog entry in banner in uswds-site.
Also need to update interactive components spreadsheet 🔒

Preview link

Preview link: Banner init test

Problem statement

Banner should initialize without user having to import and init accordion.
Banner should work independently, meaning one shouldn't trigger another
Banner shouldn't initialize or have any effect on USA Accordion

Solution

Banner JS now imports accordion [4550a41].
Created a setting (hasInit - boolean) in accordion to check if accordion.init() has run.
Call accordion.init() in banner if it hasn't. Allows us to import Banner modularly without user having to import and initialize accordion.
Removed accordion import dependency in unit test [41144b0].
~~Banner only initializes accordion on individual banners.~~

Alternatives

Remove usa-accordion dependency and use toggle.js utility to recreate functionality.
Update banner guidance to note accordion dependency.
Run accordion.on() on banner init. This will also init accordions on the page, which will cause issues for users.
Import toggle and selectors from accordion. We'd be recreating accordion functionality. More code reuse, but no real performance benefit.

Major changes

Banner script now imports accordion. Meaning users can follow Guidance to import banner via

import banner from "@uswds/uswds/js/usa-banner"

Initialize it with banner.on() and have component work properly. There are some new changes behind the scenes to check if accordion has initialized.

Testing and review

Traditional site

Check out testing branch jm-banner-init-test
Start StorybookJS npm start
Visit Banner init test page
Accordion & banner should work without any issues
Accordion should only initialize once
Locally, visit uswds-core/src/js/start.js
Init Banner only, everything should work
Init Accordion only, everything should work
Init all components, components should work without conflicts
Run npm run test:unit; banner & accordion should only contain their respective scripts & run without errors

Frameworks

Visit uswds/uswds-sandbox at jm-test-uswds-5179
Run npm install
Run npm start
Accordion and banner should work without conflicts.
Import only banner; component should initialize accordion.
Import both banner and accordion. Both components should work without conflicts.

Additional information

Additional instructions in start.js 21-27
Created branch in uswds-sandbox [jm-test-uswds-5179] for additional testing

Previous testing instructions

Keeping for historical record.

Visit preview link for Banner init test
Each banner should work
One banner should have no effect on another
Accordion component should not work. If so, it would mean that banner is unexpectedly initializing accordions. In projects this would cause unintended side effects.
npm run test:unit should run without errors.

Compiled package size

Dependency name	Previous version	New version
uswds.js	798KB	801KB
uswds.min.js	87KB	87KB

When imported as a single package, the banner won't work properly because of the missing dependency of accordion JS. - Only initialize accordion on banner buttons. Prevents side effects on accordions. - Add JSDoc comment to toggleBanner

Banners should initialize on their own.

mahoneycm

Working as described and doesn't initiate the accordion! Thanks for making the test page 👍

aduth · 2023-04-18T14:57:58Z

packages/usa-banner/src/index.js

+      bannerButtons.forEach((button) => {
+        const parentBanner = button.closest(accordion.ACCORDION);
+
+        accordion.on(parentBanner);


Does this risk being double-initialized if the accordion script is also initialized? Imagining if there might be side-effects or at least redundant event handlers.

~~Hey @aduth, the initial solution just called accordion.on() on init and it was initializing accordions as well. That's been changed.~~

~~Right now it only initializes accordions in banners.~~

Sorry, misunderstood. Yes, it gets initialized again when accordion is imported.

I've moved this PR to draft to rethink this approach.

mejiaj · 2023-04-19T18:46:33Z

Wanted to share, the test template was created with other pre-existing components and data.

If you'd like to do the same in the future you can see how I did that here:

<h2>Banner init test</h2>

<h3>Default banner</h3>
{% include "@components/usa-banner/src/usa-banner.twig" with DefaultContent %}

<h3>Default Spanish banner</h3>
{% include "@components/usa-banner/src/usa-banner.twig" with DefaultContentLangEs %}

<h3>Accordion</h3>
{% include "@components/usa-accordion/src/usa-accordion.twig" with accordionContent %}

https://github.com/uswds/uswds/blob/36d31a1b6b846aa682b7ee938b9d4ca0cfaf8636/packages/usa-banner/src/test/test-patterns/test-usa-banner-init.twig

In the future I'd like to improve the shorthand for twig components, but not a major priority right now.

amyleadem

Looks good to me. I successfully performed the test described in the PR description.
I really appreciate the well thought-out test page and guidance in this PR. It helped me better understand the problem you were solving. Thanks for the extra work there, @mejiaj!

mejiaj · 2023-04-21T18:25:43Z

packages/usa-banner/src/index.js

 const toggleBanner = function toggleEl(event) {
  event.preventDefault();
-  this.closest(HEADER).classList.toggle(EXPANDED_CLASS);
+  event.target.closest(HEADER).classList.toggle(EXPANDED_CLASS);


Using event.target instead of this to avoid potential scoping issues.

mejiaj · 2023-04-21T18:27:46Z

Re-requesting review. Unfortunately couldn't separate banner & accordion (see PR notes). But now they can be imported individually without conflicts.

There's a new solution for JS conflicts

mahoneycm

Everything is working great! Tested by:

Switching order of component initialization,
Initializing only banner/accordion
Running test

Everything worked as expected!

Should we consider making hasInit a pattern for dynamic components? In #5208 I run a conditional check to usa-modal for a class that is created during initialization, but if we wanted to be consistent I could use hasInit instead!

aduth · 2023-04-26T17:03:23Z

packages/usa-accordion/src/index.js

-        toggleButton(button, expanded);
-      });
+      // Check if Banner has previously initialized accordion.
+      if (!this.hasInit) {


Is this something which could make sense to implement as common in the behavior function initialization for all components?

I'm conflicted on this and would prefer to limit this feature for now. Mainly to test and be able to have the ability to quickly change if needed.

That said, I'm also looking forward to the teams feedback. If it's something we should move forward with on all components, that's totally possible too.

I'm comfortable with leaving this scoped to these specific components for now. But I agree it could make sense to move into behavior as we identify the need.

mejiaj · 2023-04-28T14:31:56Z

Everything is working great! Tested by:

Switching order of component initialization,

Initializing only banner/accordion

Running test

Everything worked as expected!

Should we consider making hasInit a pattern for dynamic components? In #5242 I run a conditional check to usa-modal for a class that is created during initialization, but if we wanted to be consistent I could use hasInit instead!

Hey @mahoneycm, thanks for the feedback. What's the issue/PR number for modal check? Yes, if components require it this could be a pattern moving forward, but open to ideas too.

mahoneycm · 2023-04-28T16:15:59Z

@mejiaj Apologies! Linked the wrong PR. It's #5208

mejiaj · 2023-05-02T18:26:39Z

@mahoneycm this could help resolve that problem, but I'd like to understand first why the modal component is initializing multiple times. For example, with two modals it initializing four times instead of the expected two.

I hesitate to introduce this as a fix for what might be a framework specific issue in implementation.

amyleadem

I was able to successfully perform the tests outlined in the PR description. Everything is working as expected.

This reverts commit 10ef49f.

Revert "Merge pull request #5242 from uswds/jm-banner-init"

amyleadem · 2023-09-15T21:34:07Z

According to Slack, this PR was reverted because of some JS issues when bundled.

Seems to be our bundler browserify is changing this to void(0) for some reason.

@mejiaj Does it make sense to re-open this effort?

mejiaj added 3 commits April 17, 2023 13:02

Use accordion JS in banner

4550a41

Require accordion JS in banner

64809be

When imported as a single package, the banner won't work properly because of the missing dependency of accordion JS. - Only initialize accordion on banner buttons. Prevents side effects on accordions. - Add JSDoc comment to toggleBanner

Remove accordion script from unit test

41144b0

Banners should initialize on their own.

mejiaj marked this pull request as ready for review April 17, 2023 21:12

mejiaj requested review from thisisdano, mahoneycm and amyleadem April 17, 2023 21:12

mahoneycm previously approved these changes Apr 18, 2023

View reviewed changes

aduth reviewed Apr 18, 2023

View reviewed changes

amyleadem previously approved these changes Apr 19, 2023

View reviewed changes

mejiaj marked this pull request as draft April 20, 2023 14:55

Add check for accordion initialization

79d626d

mejiaj commented Apr 21, 2023

View reviewed changes

mejiaj requested review from aduth, amyleadem and mahoneycm April 21, 2023 18:26

mejiaj marked this pull request as ready for review April 21, 2023 18:26

mahoneycm approved these changes Apr 26, 2023

View reviewed changes

aduth reviewed Apr 26, 2023

View reviewed changes

amyleadem approved these changes May 11, 2023

View reviewed changes

thisisdano approved these changes May 25, 2023

View reviewed changes

thisisdano merged commit 10ef49f into develop May 25, 2023
3 checks passed

thisisdano deleted the jm-banner-init branch May 25, 2023 21:53

thisisdano mentioned this pull request May 25, 2023

Add changelog items for USWDS 3.5.0 uswds/uswds-site#2096

Merged

24 tasks

mejiaj mentioned this pull request Jun 5, 2023

USWDS - Bug: Bundling JS causes init issues for Banner/Accordion #5322

Closed

4 tasks

thisisdano mentioned this pull request Jun 6, 2023

USWDS 3.5.0 #5323

Merged

mejiaj added a commit that referenced this pull request Jun 7, 2023

Revert "Merge pull request #5242 from uswds/jm-banner-init"

dd0c518

This reverts commit 10ef49f.

thisisdano added a commit that referenced this pull request Jun 7, 2023

Merge pull request #5326 from uswds/jm-revert-banner-init

8736f93

Revert "Merge pull request #5242 from uswds/jm-banner-init"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USWDS - Banner: Add accordion dependency #5242

USWDS - Banner: Add accordion dependency #5242

mejiaj commented Apr 17, 2023 •

edited

mahoneycm left a comment

aduth Apr 18, 2023

mejiaj Apr 18, 2023 •

edited

mejiaj Apr 20, 2023

mejiaj commented Apr 19, 2023 •

edited

amyleadem left a comment

mejiaj Apr 21, 2023

mejiaj commented Apr 21, 2023 •

edited

mahoneycm left a comment •

edited

aduth Apr 26, 2023

mejiaj Apr 28, 2023

thisisdano May 25, 2023

mejiaj commented Apr 28, 2023

mahoneycm commented Apr 28, 2023

mejiaj commented May 2, 2023

amyleadem left a comment

amyleadem commented Sep 15, 2023

USWDS - Banner: Add accordion dependency #5242

USWDS - Banner: Add accordion dependency #5242

Conversation

mejiaj commented Apr 17, 2023 • edited

Summary

Breaking change

Related issue

Related pull requests

Preview link

Problem statement

Solution

Alternatives

Major changes

Testing and review

Additional information

Compiled package size

mahoneycm left a comment

Choose a reason for hiding this comment

aduth Apr 18, 2023

Choose a reason for hiding this comment

mejiaj Apr 18, 2023 • edited

Choose a reason for hiding this comment

mejiaj Apr 20, 2023

Choose a reason for hiding this comment

mejiaj commented Apr 19, 2023 • edited

amyleadem left a comment

Choose a reason for hiding this comment

mejiaj Apr 21, 2023

Choose a reason for hiding this comment

mejiaj commented Apr 21, 2023 • edited

mahoneycm left a comment • edited

Choose a reason for hiding this comment

aduth Apr 26, 2023

Choose a reason for hiding this comment

mejiaj Apr 28, 2023

Choose a reason for hiding this comment

thisisdano May 25, 2023

Choose a reason for hiding this comment

mejiaj commented Apr 28, 2023

mahoneycm commented Apr 28, 2023

mejiaj commented May 2, 2023

amyleadem left a comment

Choose a reason for hiding this comment

amyleadem commented Sep 15, 2023

mejiaj commented Apr 17, 2023 •

edited

mejiaj Apr 18, 2023 •

edited

mejiaj commented Apr 19, 2023 •

edited

mejiaj commented Apr 21, 2023 •

edited

mahoneycm left a comment •

edited