Skip to content

Conversation

@markconroy
Copy link
Member

Closes #811

@markconroy markconroy requested a review from ctorgalson June 24, 2025 10:14
Copy link
Contributor

@ctorgalson ctorgalson left a comment

Choose a reason for hiding this comment

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

Late review, sorry.

TL;DR: I think this is good but needs a guard clause to complete #811; I didn't have a chance to test it.

Using .once() will only return DOM elements once, but I think the behavior will still run multiple times.

The second part we usually add is one or more guard clauses (see suggestion). This makes sure that, on subsequent runs, the behavior doesn't try to do anything and exits immediately.

For example, if this behavior ran twice, the first go would mark the menu and toggle with a data-once attribute and continue through the code. But on the second iteration, I think the two variables will be undefined, and line 17 will try to add an event listener to an undefined element.

);
const subsitesMenu = document.querySelector('.subsite-extra-menu');
const [subsitesMenuToggle] = once('allSubsitesMenuToggles', '.subsite-extra__header-toggle-button', context);
const [subsitesMenu] = once('allSubsitesMenuToggles', '.subsite-extra-menu', context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [subsitesMenu] = once('allSubsitesMenuToggles', '.subsite-extra-menu', context);
const [subsitesMenu] = once('allSubsitesMenuToggles', '.subsite-extra-menu', context);
if (!subsitesMenuToggle || !subsitesMenu) {
return;
}

This is untested, but it's the usual defensive companion to .once().

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @ctorgalson, I've added in the extra defensive clause now. Let me know if you think we're missing any thinbg.

@finnlewis finnlewis merged commit e2ca32a into 2.x Jul 8, 2025
12 of 17 checks passed
@finnlewis
Copy link
Member

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.

Use context + once instead of document + querySelector in subsites-menu.js

4 participants