Thet/4321 inject scroll #1269
Thet/4321 inject scroll #1269
Conversation
thet
force-pushed
the
thet/4321--inject-scroll
branch
4 times, most recently
from
8f47dec to
6ba1501
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue with the inject pattern's scroll functionality (scrum 4321) by improving how scroll targets are located within injected content. The main fix allows querySelectorAllAndMe to accept multiple elements (jQuery objects, arrays) instead of just a single element, enabling proper scroll target selection when multiple elements are injected.
Key changes:
- Added
to_element_arrayfunction to filter DOM nodes to only Elements (not text nodes or other node types) - Enhanced
querySelectorAllAndMeto support multiple root elements instead of just a single element - Fixed scroll target selection in inject pattern to work with multiple injected elements
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/dom.js | Refactored toNodeArray to to_node_array, added new to_element_array function, and enhanced querySelectorAllAndMe to accept multiple root elements for searching |
| src/core/dom.test.js | Added comprehensive test coverage for the new to_element_array function and enhanced querySelectorAllAndMe functionality with multiple root elements |
| src/core/utils.js | Renamed parameter from nodes to elements in hideOrShow function and fixed indentation formatting |
| src/pat/inject/inject.js | Fixed bug where scroll target was only searched in first injected element; now searches across all injected elements |
| src/pat/inject/inject.test.js | Added two new test cases (9.1.13 and 9.1.14) to verify scroll functionality with single and multiple injected elements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thet
force-pushed
the
thet/4321--inject-scroll
branch
from
6ba1501 to
b4c02cf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (nodes instanceof Array === false) { | ||
| nodes = [nodes]; |
There was a problem hiding this comment.
When nodes is undefined or null, the condition nodes instanceof Array === false evaluates to true, which wraps the falsy value in an array [undefined] or [null]. While the filter on line 51 removes these values, it's cleaner and more explicit to handle falsy values upfront. Consider adding a guard: if (!nodes) { return []; } at the beginning of the function.
| const all = []; | ||
|
|
||
| for (const root of roots) { | ||
| if (root.matches(selector) && !seen.has(root)) { |
There was a problem hiding this comment.
When selector is undefined or null and there are root elements, calling root.matches(selector) on line 85 will throw a TypeError. Consider adding a guard check: if (!selector) { return []; } at the beginning of the function to handle this edge case gracefully.
| /** | ||
| * Return an array of DOM elements. | ||
| * | ||
| * @param {Node|NodeList|jQuery} nodes - The object which should be returned as array. |
There was a problem hiding this comment.
The JSDoc parameter type lists Node but the function filters for Element instances only. The parameter description should be {Element|NodeList|jQuery} or {HTMLElement|NodeList|jQuery} to accurately reflect that only Elements (not all Nodes like text nodes) are returned.
| * @param {Node|NodeList|jQuery} nodes - The object which should be returned as array. | |
| * @param {Element|NodeList|jQuery} nodes - The object which should be returned as array. |
This method can be passed a single object, a NodeList or an array and will return an array, filtered for DOM elements.
…name for BBB. This is done to align the names according the naming conventions in Patternslib. Also, this is not a breaking change, as toNodeArray is now nowhere used (we're using to_element_array instead) and we kept the old name as alias for backwards compatibility.
querySelectorAllAndMe supports now multiple root nodes. All of them are searched for a selector match.
Return the results in the order of the matching root elements, extended their matching containing elements. Note: not a breaking change, as the support for multiple root nodes was added in this release.
This fixes a problem where scrolling wasn't working after injection anymore when the injection result is multiple nodes and not a single one.
Use dom.to_element_array instead of dom.toNodeArray to filter for DOM elements instead of nodes.
thet
force-pushed
the
thet/4321--inject-scroll
branch
from
b4c02cf to
c20c58a
Compare
Fixes scrum 4321