Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts how code block height is constrained by moving the max-height styling from the Pre component’s inline style to the DemoBlock card footer CSS, while normalizing some CSS file headers. 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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor.Server/Components/Components/Pre.razor.cs" line_range="78" />
<code_context>
private string? StyleString => CssBuilder.Default()
.AddClass($"height: {Height};", !string.IsNullOrEmpty(Height))
- .AddClass("max-height: 260px;", string.IsNullOrEmpty(Height))
.AddClassFromAttributes(AdditionalAttributes)
.Build();
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing the default max-height may affect Pre usage outside DemoBlock contexts.
`Pre` used to enforce `max-height: 260px` whenever `Height` was unset, in all contexts. Now that constraint only exists via `.card-footer-code ::deep pre` in `DemoBlock.razor.css`, so any other `Pre` usages without an explicit `Height` can now expand indefinitely. If that global behavior change isn’t desired, consider retaining a default `max-height` in `Pre` and overriding it specifically in `DemoBlock` instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| private string? StyleString => CssBuilder.Default() | ||
| .AddClass($"height: {Height};", !string.IsNullOrEmpty(Height)) | ||
| .AddClass("max-height: 260px;", string.IsNullOrEmpty(Height)) |
There was a problem hiding this comment.
issue (bug_risk): Removing the default max-height may affect Pre usage outside DemoBlock contexts.
Pre used to enforce max-height: 260px whenever Height was unset, in all contexts. Now that constraint only exists via .card-footer-code ::deep pre in DemoBlock.razor.css, so any other Pre usages without an explicit Height can now expand indefinitely. If that global behavior change isn’t desired, consider retaining a default max-height in Pre and overriding it specifically in DemoBlock instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7723 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 749 749
Lines 33207 33207
Branches 4604 4604
=========================================
Hits 33207 33207
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:
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts how the Pre code block’s default max-height is applied in the BootstrapBlazor.Server docs site, moving the constraint from component inline styles to demo-block scoped CSS.
Changes:
- Removed the default
max-height: 260px;inline style behavior fromPrewhenHeightis not set. - Added a demo-specific
max-height: 260px;rule for code blocks rendered insideDemoBlock’s footer area. - Updated the first line of two scoped CSS files (appears to remove UTF-8 BOM / change encoding).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor.Server/Components/Layout/ComponentLayout.razor.css | Encoding/first-line change only. |
| src/BootstrapBlazor.Server/Components/Components/Pre.razor.cs | Removes default max-height from StyleString. |
| src/BootstrapBlazor.Server/Components/Components/DemoBlock.razor.css | Adds max-height styling for demo code footer; encoding/first-line change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private string? StyleString => CssBuilder.Default() | ||
| .AddClass($"height: {Height};", !string.IsNullOrEmpty(Height)) | ||
| .AddClass("max-height: 260px;", string.IsNullOrEmpty(Height)) | ||
| .AddClassFromAttributes(AdditionalAttributes) |
There was a problem hiding this comment.
StyleString is used for the <pre> element’s style attribute, but it currently calls AddClassFromAttributes(AdditionalAttributes), which pulls the class attribute (not style) and appends it into the style string. This also means any user-provided style in AdditionalAttributes is ignored for the <pre>. Use AddStyleFromAttributes(AdditionalAttributes) here (consistent with other components like Scroll/Affix).
| .AddClassFromAttributes(AdditionalAttributes) | |
| .AddStyleFromAttributes(AdditionalAttributes) |
| @@ -1,4 +1,4 @@ | |||
| .bb-title { | |||
| .bb-title { | |||
There was a problem hiding this comment.
This file now starts without the UTF-8 BOM marker, while most .razor.css files in this project appear to keep the BOM (e.g., many other component/layout scoped CSS files). If the change is unintentional, consider restoring the BOM to avoid inconsistent encoding/noisy diffs across the repo.
| @@ -1,4 +1,4 @@ | |||
| .card { | |||
| .card { | |||
There was a problem hiding this comment.
This file now starts without the UTF-8 BOM marker, while most .razor.css files in this project appear to keep the BOM. If the change is unintentional, consider restoring the BOM to keep encoding consistent and reduce churn in diffs.
Link issues
fixes #7722
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enhancements: