-
-
Notifications
You must be signed in to change notification settings - Fork 363
feat(Link):添加 AddToHead 参数
#7092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for your PR, @AiYuZhen. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Reviewer's GuideAdds an IsAddToHead flag to the Link component enabling optional insertion of tags into the document head via a JS module, avoids duplicate tags, and updates unit tests. Sequence diagram for Link component rendering with IsAddToHead=truesequenceDiagram
participant LinkComponent
participant Browser
participant JSModule
LinkComponent->>Browser: Render (firstRender)
LinkComponent->>JSModule: Invoke init({Href, Rel})
JSModule->>Browser: Check for existing <link> in head
alt Link not found
JSModule->>Browser: Append new <link> to head
else Link found
JSModule->>Browser: Do nothing
end
Class diagram for updated Link componentclassDiagram
class Link {
+string? Version
+bool IsAddToHead
+IVersionService VersionService
+string GetHref()
+Task OnAfterRenderAsync(bool firstRender)
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider renaming IsAddToHead to a name like AddToHead or AppendToHead to better follow .NET boolean property naming conventions.
- Add disposal or parameter‐change logic to remove or update injected link tags when the component is unmounted or its parameters change to prevent stale entries.
- In the JavaScript init function, use the URL API (e.g. new URL(href, document.baseURI)) instead of string concatenation for more robust href comparisons and to support relative URLs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming IsAddToHead to a name like AddToHead or AppendToHead to better follow .NET boolean property naming conventions.
- Add disposal or parameter‐change logic to remove or update injected link tags when the component is unmounted or its parameters change to prevent stale entries.
- In the JavaScript init function, use the URL API (e.g. new URL(href, document.baseURI)) instead of string concatenation for more robust href comparisons and to support relative URLs.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/HtmlTag/Link.razor.js:2-3` </location>
<code_context>
+export function init(options) {
+ const href = options.href;
+ const rel = options.rel;
+ const links = document.head.getElementsByTagName("link");
+ // 遍历所有link元素
</code_context>
<issue_to_address>
**issue:** Consider normalizing option property casing for consistency.
The C# object uses 'Href' and 'Rel', while the JS expects 'href' and 'rel'. Please align property names to prevent undefined values.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/HtmlTag/Link.razor.js:8` </location>
<code_context>
+ // 遍历所有link元素
+ for (let i = 0; i < links.length; i++) {
+ const hlink = links[i];
+ var nhref = hlink.baseURI + href;
+ if (hlink.href == nhref && hlink.rel == rel) {
+ return;
</code_context>
<issue_to_address>
**issue (bug_risk):** The construction of nhref may not match the actual href attribute value.
Directly concatenating baseURI and href can lead to incorrect URLs if href is absolute or already includes the base URI. Use proper URL resolution or compare hlink.href and href directly.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/HtmlTag/Link.razor.js:2` </location>
<code_context>
const href = options.href;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {href} = options;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Components/HtmlTag/Link.razor.js:3` </location>
<code_context>
const rel = options.rel;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {rel} = options;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Components/HtmlTag/Link.razor.js:8` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
IsAddToHead 参数,支持将 link 标签添加到 head 元素中。IsAddToHead 参数
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7092 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 746 +1
Lines 32556 32568 +12
Branches 4512 4514 +2
=========================================
+ Hits 32556 32568 +12
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:
|
|
通过 |
#7091
Link issues
fixes #{issue number}
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable Link components to inject tags into the page head through a new IsAddToHead parameter and a custom JS interop module, while preserving existing rendering behavior when the feature is not enabled.
New Features:
Enhancements:
Tests: