Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds an optional File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- With the new
allflag,getDescribedElementnow sometimes returns aNodeListinstead of a single element; consider either keeping the return type consistent (e.g., always returning an array) or auditing and updating all call sites to handle both cases explicitly. - The
allparameter name is a bit unclear at the call site; consider renaming it to something more explicit likereturnAllormultipleto better convey that it switches between a single element and aNodeList.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- With the new `all` flag, `getDescribedElement` now sometimes returns a `NodeList` instead of a single element; consider either keeping the return type consistent (e.g., always returning an array) or auditing and updating all call sites to handle both cases explicitly.
- The `all` parameter name is a bit unclear at the call site; consider renaming it to something more explicit like `returnAll` or `multiple` to better convey that it switches between a single element and a `NodeList`.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/wwwroot/modules/utility.js" line_range="423-430" />
<code_context>
}
-const getDescribedElement = (element, selector = 'aria-describedby') => {
+const getDescribedElement = (element, selector = 'aria-describedby', all = false) => {
if (isElement(element)) {
let id = element.getAttribute(selector)
if (id) {
if (id.indexOf('.') === -1) {
id = `#${id}`
}
- return document.querySelector(id)
+ return all ? document.querySelectorAll(id) : document.querySelector(id)
}
}
</code_context>
<issue_to_address>
**issue:** Handling multiple aria-describedby IDs with `all` is likely incorrect with the current selector construction.
With `all = true`, this now does `querySelectorAll(id)`, but `aria-describedby` often holds a space-separated list of IDs (e.g. `"id1 id2"`). That makes `id` become `"#id1 id2"`, which is parsed as a descendant selector, not multiple IDs, so it won’t reliably return all referenced elements. If `all` is meant to return all described elements, you likely need to split on whitespace, prefix each with `#`, and either build a comma-separated selector for `querySelectorAll` or use `getElementById` per ID and aggregate the results.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const getDescribedElement = (element, selector = 'aria-describedby', all = false) => { | ||
| if (isElement(element)) { | ||
| let id = element.getAttribute(selector) | ||
| if (id) { | ||
| if (id.indexOf('.') === -1) { | ||
| id = `#${id}` | ||
| } | ||
| return document.querySelector(id) | ||
| return all ? document.querySelectorAll(id) : document.querySelector(id) |
There was a problem hiding this comment.
issue: Handling multiple aria-describedby IDs with all is likely incorrect with the current selector construction.
With all = true, this now does querySelectorAll(id), but aria-describedby often holds a space-separated list of IDs (e.g. "id1 id2"). That makes id become "#id1 id2", which is parsed as a descendant selector, not multiple IDs, so it won’t reliably return all referenced elements. If all is meant to return all described elements, you likely need to split on whitespace, prefix each with #, and either build a comma-separated selector for querySelectorAll or use getElementById per ID and aggregate the results.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in capability for getDescribedElement (utility JS helper) to return multiple matched elements, and bumps the package version for the next beta release.
Changes:
- Extend
getDescribedElementwith anallparameter that switches betweenquerySelectorandquerySelectorAll. - Bump
BootstrapBlazorpackage version from10.4.2-beta01to10.4.2-beta02.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/BootstrapBlazor/wwwroot/modules/utility.js | Adds all parameter to getDescribedElement to optionally return multiple matches. |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Updates package version to 10.4.2-beta02. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return document.querySelector(id) | ||
| return all ? document.querySelectorAll(id) : document.querySelector(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
getDescribedElement now returns different types depending on all (Element vs NodeList vs null). This makes the API easy to misuse (e.g., if (ret) ret.querySelector(...) works for Element but breaks for NodeList; also querySelectorAll returns an empty NodeList which is truthy, while the function otherwise returns null). Consider keeping the return type consistent: either split into getDescribedElement (single) + getDescribedElements (all), or when all=true always return a (possibly empty) array/NodeList and never null, and document the contract clearly for callers.
| } | |
| } | |
| if (all) { | |
| // Always return a NodeList for the "all" mode, even when there is no match, | |
| // to mirror querySelectorAll semantics and keep the return type consistent. | |
| return document.querySelectorAll('[data-utility-empty-aria-describedby=""]') | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7778 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 33960 33960
Branches 4675 4675
=========================================
Hits 33960 33960
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #7777
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
New Features: