Conversation
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
Contributor
Reviewer's GuideExtends the Mask component options with an AppendToBody flag that controls whether the mask element is appended to document.body, wires this option through the .NET and JS layers, and ensures the mask instance unregisters from MaskService on disposal. Sequence diagram for Mask update with AppendToBody optionsequenceDiagram
actor User
participant Mask as MaskComponent
participant MaskService
participant JS as JSInterop
participant Script as Mask_razor_js
participant DOM
User->>MaskService: RequestMask(option)
MaskService-->>Mask: Show(option)
Mask->>Mask: _options = option
Mask->>JS: InvokeVoidAsync(update, id, { show, containerId, selector, appendToBody })
JS-->>Script: update(id, options)
Script->>DOM: const mask = getElementById(id)
alt mask exists
Script->>DOM: el = querySelector([data-bb-mask=id])
Script->>DOM: container = getContainerBySelector(options)
alt container exists
Script->>DOM: container.appendChild(el)
else appendToBody is true
Script->>DOM: document.body.appendChild(el)
else appendToBody is false
Script-->>DOM: no reparenting
end
else mask does not exist
Script-->>JS: return
end
User->>Mask: DisposeAsync(disposing: true)
Mask->>MaskService: UnRegister(this)
Mask->>JS: Dispose module via base.DisposeAsync
Updated class diagram for Mask and MaskOption with AppendToBody and disposalclassDiagram
class BootstrapModuleComponentBase
class Mask {
- MaskOption _options
+ Task Show(MaskOption option)
+ Task CloseAsync()
+ ValueTask DisposeAsync(bool disposing)
}
class MaskOption {
+ string ContainerId
+ string Selector
+ bool AppendToBody = true
}
class MaskService {
+ void Register(Mask mask)
+ void UnRegister(Mask mask)
}
Mask --|> BootstrapModuleComponentBase
Mask --> MaskOption : uses
MaskService ..> Mask : manages
File-Level Changes
Assessment against 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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MaskOption.AppendToBodyXML docs, the<vesion>tag appears to be misspelled and may not be recognized by your tooling; consider correcting or removing it. - The override of
DisposeAsync(bool disposing)mixes<inheritdoc/>inside<summary>and has an empty<param>tag; consider simplifying the XML comment (e.g., just<inheritdoc />) to avoid misleading or redundant documentation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MaskOption.AppendToBody` XML docs, the `<vesion>` tag appears to be misspelled and may not be recognized by your tooling; consider correcting or removing it.
- The override of `DisposeAsync(bool disposing)` mixes `<inheritdoc/>` inside `<summary>` and has an empty `<param>` tag; consider simplifying the XML comment (e.g., just `<inheritdoc />`) to avoid misleading or redundant documentation.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Components/Mask/MaskOption.cs" line_range="51-55" />
<code_context>
+ /// <summary>
+ /// <para lang="zh">获得/设置 是否将遮罩追加到 body 元素 默认 true</para>
+ /// <para lang="en">Gets or sets whether to append the mask to the body element. Default true</para>
+ /// <para>v<vesion>10.5.1</vesion></para>
+ /// </summary>
+ public bool AppendToBody { get; set; } = true;
}
</code_context>
<issue_to_address>
**suggestion (typo):** Fix the `<vesion>` tag typo and redundant leading `v` in the XML doc.
The `vesion` tag appears to be a typo, and the leading `v` makes the XML odd. Use a proper tag (e.g. `<version>10.5.1</version>`) that your tooling recognizes so XML parsers and doc generators can handle it correctly.
```suggestion
/// <para lang="zh">获得/设置 是否将遮罩追加到 body 元素 默认 true</para>
/// <para lang="en">Gets or sets whether to append the mask to the body element. Default true</para>
/// <para><version>10.5.1</version></para>
/// </summary>
public bool AppendToBody { get; set; } = true;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+55
| /// <para lang="zh">获得/设置 是否将遮罩追加到 body 元素 默认 true</para> | ||
| /// <para lang="en">Gets or sets whether to append the mask to the body element. Default true</para> | ||
| /// <para>v<vesion>10.5.1</vesion></para> | ||
| /// </summary> | ||
| public bool AppendToBody { get; set; } = true; |
Contributor
There was a problem hiding this comment.
suggestion (typo): Fix the <vesion> tag typo and redundant leading v in the XML doc.
The vesion tag appears to be a typo, and the leading v makes the XML odd. Use a proper tag (e.g. <version>10.5.1</version>) that your tooling recognizes so XML parsers and doc generators can handle it correctly.
Suggested change
| /// <para lang="zh">获得/设置 是否将遮罩追加到 body 元素 默认 true</para> | |
| /// <para lang="en">Gets or sets whether to append the mask to the body element. Default true</para> | |
| /// <para>v<vesion>10.5.1</vesion></para> | |
| /// </summary> | |
| public bool AppendToBody { get; set; } = true; | |
| /// <para lang="zh">获得/设置 是否将遮罩追加到 body 元素 默认 true</para> | |
| /// <para lang="en">Gets or sets whether to append the mask to the body element. Default true</para> | |
| /// <para><version>10.5.1</version></para> | |
| /// </summary> | |
| public bool AppendToBody { get; set; } = true; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7834 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34147 34165 +18
Branches 4701 4704 +3
=========================================
+ Hits 34147 34165 +18
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Mask component API and related implementation to support controlling whether the mask element is appended to document.body, along with some lifecycle and packaging adjustments.
Changes:
- Add
AppendToBodyoption toMaskOptionand pass it through Blazor → JS interop. - Update
Mask.razor.jsplacement logic to conditionally append tobody. - Unregister
MaskfromMaskServiceon dispose; bump package version and remove BOM/formatting artifacts in a few files.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Mask/MaskOption.cs | Adds AppendToBody option (with XML doc update). |
| src/BootstrapBlazor/Components/Mask/Mask.razor.js | Updates DOM placement logic based on appendToBody. |
| src/BootstrapBlazor/Components/Mask/Mask.razor.cs | Passes new option to JS and unregisters from MaskService on dispose. |
| src/BootstrapBlazor/Components/Mask/Mask.razor | Removes BOM/whitespace artifact (no functional change). |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Bumps library version. |
| src/BootstrapBlazor.Server/Components/Samples/Masks.razor.cs | Removes BOM/whitespace artifact (no functional change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// <para lang="zh">获得/设置 是否将遮罩追加到 body 元素 默认 true</para> | ||
| /// <para lang="en">Gets or sets whether to append the mask to the body element. Default true</para> | ||
| /// <para>v<vesion>10.5.1</vesion></para> |
There was a problem hiding this comment.
XML doc tag appears misspelled: <vesion> isn’t used elsewhere in the codebase (convention is <version>). This will likely break doc tooling / consistency; please change to <version>.
Suggested change
| /// <para>v<vesion>10.5.1</vesion></para> | |
| /// <para>v<version>10.5.1</version></para> |
Comment on lines
+17
to
19
| else if (appendToBody === true) { | ||
| document.body.appendChild(el); | ||
| } |
There was a problem hiding this comment.
When appendToBody is false and no container is resolved, the mask element stays inside the <template> and therefore won’t be rendered/visible even when show is true. Consider appending to a sensible default (e.g., mask.parentElement) or enforcing that containerId/selector must be set when appendToBody is false (with a clear error).
Comment on lines
3
to
5
| <PropertyGroup> | ||
| <Version>10.5.1-beta01</Version> | ||
| <Version>10.5.1-beta04</Version> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
PR title/linked issue indicate a documentation-only change, but this PR also introduces behavioral changes (new AppendToBody option, JS logic changes) and bumps the package version. Please align the PR metadata/title/description with the actual scope, or split version bump/behavior changes into a separate PR.
ArgoZhang
changed the title
ArgoZhang
added
enhancement
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7833
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a configurable option for Mask placement and improve its lifecycle cleanup with service unregistration.
New Features:
Enhancements:
Chores: