Conversation
Reviewer's GuideAdds a configurable print button to the PdfReader component, replaces the boolean fit-to-page API with a more flexible PdfReaderFitMode enum, and refactors the toolbar UI/JS interop to support multiple fit modes and improved dropdown controls. Sequence diagram for PdfReader fit mode change via toolbarsequenceDiagram
actor User
participant PdfReaderComponent
participant BlazorRenderer
participant JsInterop as PdfReaderJsInterop
participant PdfViewer as PdfJsPdfViewer
User->>PdfReaderComponent: Click fit mode button
PdfReaderComponent->>PdfReaderComponent: SetFitMode(mode)
PdfReaderComponent->>BlazorRenderer: Request re-render
BlazorRenderer->>PdfReaderComponent: OnAfterRenderAsync(firstRender=false)
PdfReaderComponent->>PdfReaderComponent: Detect _fitMode != FitMode
PdfReaderComponent->>PdfReaderComponent: _fitMode = FitMode
PdfReaderComponent->>JsInterop: InvokeVoidAsync("setScaleValue", Id, FitMode.ToDescriptionString())
JsInterop->>PdfViewer: setScaleValue(id, value)
PdfViewer->>PdfViewer: pdfViewer.currentScaleValue = value
Class diagram for updated PdfReader component and PdfReaderFitMode enumclassDiagram
class PdfReader {
+bool ShowDownload
+bool ShowPrint
+bool EnableThumbnails
+string? Url
+uint CurrentPage
+string? CurrentScale
+PdfReaderFitMode FitMode
+bool ShowTwoPagesOneView
+bool EnableTwoPagesOneView
+bool EnableMonitorDocumentState
+string? MoreButtonIcon
+Func~Task~? OnPagesInitAsync
+Func~int,Task~? OnPagesLoadedAsync
+Func~uint,Task~? OnPageChangedAsync
+Func~Task~? OnDownloadAsync
-string? ClassString
-string? ToolbarClassString
-string? ViewBodyString
-string? _docTitle
-PdfReaderFitMode _fitMode
-uint _currentPage
-string? _url
-string? _currentScale
-bool _enableTwoPagesOneView
-bool _showTwoPagesOneViewButton
-string? _twoPagesOneViewIcon
+void SetFitMode(mode PdfReaderFitMode)
+void RotateLeft()
+void RotateRight()
+Task OnDownload()
+Task InvokeInitAsync()
+Task OnAfterRenderAsync(firstRender bool)
+void OnParametersSet()
}
class PdfReaderFitMode {
<<enumeration>>
PageWidth
PageActual
PageHeight
PageFit
Auto
}
PdfReader --> PdfReaderFitMode : uses
PdfReader "1" o--> "1" PdfReaderFitMode : FitMode
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 there - I've reviewed your changes and found some issues that need to be addressed.
- In
ViewBodyString, thefit-widthCSS class is applied whenFitMode == PdfReaderFitMode.PageHeight, which looks inverted relative to the intent and the.bb-view-body.fit-widthCSS rules; consider switching this condition toPageWidth(and/or introducing a separate class for height) so the toolbar state matches the actual fit mode. - The JS init/event logic appears inconsistent with the new
FitModeparameter:initsendsFitMode(an enum) butaddEventListenerstill referencesoptions.isFitToPageand then assignspdfViewer.currentScaleValue = fitMode, wherefitModeandoptions.fitModeare undefined/incorrect casing; update the JS options shape and use the enum's description string so the initial scale is applied correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ViewBodyString`, the `fit-width` CSS class is applied when `FitMode == PdfReaderFitMode.PageHeight`, which looks inverted relative to the intent and the `.bb-view-body.fit-width` CSS rules; consider switching this condition to `PageWidth` (and/or introducing a separate class for height) so the toolbar state matches the actual fit mode.
- The JS init/event logic appears inconsistent with the new `FitMode` parameter: `init` sends `FitMode` (an enum) but `addEventListener` still references `options.isFitToPage` and then assigns `pdfViewer.currentScaleValue = fitMode`, where `fitMode` and `options.fitMode` are undefined/incorrect casing; update the JS options shape and use the enum's description string so the initial scale is applied correctly.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:128` </location>
<code_context>
private string? ViewBodyString => CssBuilder.Default("bb-view-body")
- .AddClass("fit-page", IsFitToPage)
+ .AddClass("fit-width", FitMode == PdfReaderFitMode.PageHeight)
.Build();
</code_context>
<issue_to_address>
**issue (bug_risk):** The `fit-width` CSS class is tied to `PageHeight` instead of `PageWidth`, which looks inverted.
This will mark the body as `fit-width` when `FitMode == PdfReaderFitMode.PageHeight`, so the toolbar state and CSS selectors (e.g. `.bb-view-body.fit-width .bb-view-fit-height`) won’t match the actual fit mode. Please change the condition to `FitMode == PdfReaderFitMode.PageWidth`, or use a different class if this is intended to represent height instead.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:114-115` </location>
<code_context>
- }
- else {
- pdfViewer.currentScaleValue = "page-actual";
+ if (options.fitMode) {
+ pdfViewer.currentScaleValue = fitMode;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** `fitMode` is referenced but never defined; `options.fitMode` is likely intended.
This will throw a `ReferenceError` at runtime because `fitMode` is not in scope here. Use `options.fitMode` (or a value derived from it) when setting `pdfViewer.currentScaleValue` so the intended fit behavior works correctly.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:263` </location>
<code_context>
{
Url,
- IsFitToPage,
+ FitMode,
EnableThumbnails,
TriggerPagesInit = OnPagesInitAsync != null,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Passing the raw `PdfReaderFitMode` enum to JS may not match the string values expected by `pdfViewer.currentScaleValue`.
Because `FitMode` is an enum, Blazor will serialize it as a number in the JS options, but `pdfViewer.currentScaleValue` expects string values like `"page-width"` / `"page-height"`. `setScaleValue` already uses `_fitMode.ToDescriptionString()` to produce those strings. For consistency and to avoid an incorrect initial scale, pass `FitMode.ToDescriptionString()` (or an equivalent string) into JS and have the JS use that string instead of the raw enum value.
Suggested implementation:
```csharp
protected override Task InvokeInitAsync() => InvokeVoidAsync("init", Id, Interop, new
{
Url,
FitMode = FitMode.ToDescriptionString(),
EnableThumbnails,
TriggerPagesInit = OnPagesInitAsync != null,
TriggerPagesLoaded = OnPagesLoadedAsync != null,
```
On the JavaScript side (where the `init` interop call is handled), adjust the code so that it treats the `FitMode` option as a string that is directly usable with `pdfViewer.currentScaleValue` / `setScaleValue`. For example, if it currently assumes a numeric enum and maps it to a string, remove that mapping and instead use the provided string value directly (e.g., `"page-width"`, `"page-height"`, etc.).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds a new ShowPrint parameter to control the visibility of the print button and refactors the PDF fit mode functionality from a boolean IsFitToPage parameter to a more flexible enum-based FitMode parameter. Additionally, it refactors the "more options" dropdown from a custom Dropdown component to native Bootstrap dropdown markup.
Key Changes
- Added
ShowPrintparameter to control print button visibility (default: true) - Replaced boolean
IsFitToPagewith enumPdfReaderFitModesupporting multiple fit modes (PageWidth, PageHeight, PageActual, PageFit, Auto) - Refactored toolbar UI to use Bootstrap split button dropdown for fit mode selection
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReaderFitMode.cs | New enum defining document fit modes with description attributes for JavaScript interop |
| PdfReader.razor.js | Refactored fit mode logic to use a single setScaleValue method instead of separate fitToWidth/fitToPage methods |
| PdfReader.razor.css | Updated CSS classes from fit-page to fit-width, adjusted spacing and added styles for split button dropdown |
| PdfReader.razor.cs | Added ShowPrint parameter, replaced IsFitToPage boolean with FitMode enum, renamed ShowTwoPagesOneViewButton to ShowTwoPagesOneView |
| PdfReader.razor | Added conditional rendering for print button, replaced custom Dropdown component with native Bootstrap dropdown, added split button UI for fit mode selection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -277,12 +278,7 @@ protected override async Task OnAfterRenderAsync(bool firstRender) | |||
| /// <summary> | |||
| /// 适应页面宽度 | |||
There was a problem hiding this comment.
The documentation comment "适应页面宽度" (Fit to page width) is misleading. This method now sets any fit mode via parameter, not just page width. The comment should be updated to reflect the actual functionality, e.g., "设置文档适配模式" (Set document fit mode).
Suggested change
| /// 适应页面宽度 | |
| /// 设置文档适配模式 |
| @@ -59,13 +65,13 @@ public partial class PdfReader | |||
| /// 获得/设置 是否适配当前页面宽度 默认 false | |||
There was a problem hiding this comment.
The documentation comment "获得/设置 是否适配当前页面宽度 默认 false" (Get/Set whether to fit current page width, default false) is outdated. The parameter type changed from bool IsFitToPage to PdfReaderFitMode FitMode, which accepts multiple fit modes. The comment should be updated to reflect this, e.g., "获得/设置 文档适配模式 默认 Auto" (Get/Set document fit mode, default Auto).
Suggested change
| /// 获得/设置 是否适配当前页面宽度 默认 false | |
| /// 获得/设置 文档适配模式 默认 Auto |
| else { | ||
| pdfViewer.currentScaleValue = "page-actual"; | ||
| if (options.fitMode) { | ||
| pdfViewer.currentScaleValue = fitMode; |
There was a problem hiding this comment.
The variable fitMode is undefined. It should be options.fitMode to match the condition check on line 114.
Suggested change
| pdfViewer.currentScaleValue = fitMode; | |
| pdfViewer.currentScaleValue = options.fitMode; |
| { | ||
| Url, | ||
| IsFitToPage, | ||
| FitMode, |
There was a problem hiding this comment.
The FitMode enum value is being passed directly to JavaScript, but it needs to be converted to its description string value (e.g., "page-width", "page-height"). It should be FitMode.ToDescriptionString() or a similar conversion, similar to what's done on line 232.
Suggested change
| FitMode, | |
| FitMode = FitMode.ToDescriptionString(), |
| /// <summary> | ||
| /// PdfReader 文档适配模式 | ||
| /// </summary> | ||
| public enum PdfReaderFitMode |
There was a problem hiding this comment.
The enum name PdfReaderFitMode is inconsistent with the naming convention used for other enums in this component. Other enums in the PdfReader component use the "Enum" prefix (e.g., EnumPageMode, EnumZoomMode). Consider renaming to EnumFitMode for consistency.
Suggested change
| public enum PdfReaderFitMode | |
| public enum EnumFitMode |
Comment on lines
+9
to
+43
| /// <summary> | ||
| /// PdfReader 文档适配模式 | ||
| /// </summary> | ||
| public enum PdfReaderFitMode | ||
| { | ||
| /// <summary> | ||
| /// 页面宽度 | ||
| /// </summary> | ||
| [Description("page-width")] | ||
| PageWidth, | ||
|
|
||
| /// <summary> | ||
| /// 实际大小 | ||
| /// </summary> | ||
| [Description("page-actual")] | ||
| PageActual, | ||
|
|
||
| /// <summary> | ||
| /// 页面高度 | ||
| /// </summary> | ||
| [Description("page-height")] | ||
| PageHeight, | ||
|
|
||
| /// <summary> | ||
| /// 自适应宽高 | ||
| /// </summary> | ||
| [Description("page-fit")] | ||
| PageFit, | ||
|
|
||
| /// <summary> | ||
| /// 自动 | ||
| /// </summary> | ||
| [Description("auto")] | ||
| Auto | ||
| } |
There was a problem hiding this comment.
This enum duplicates functionality already present in EnumZoomMode which has the same values: Auto, PageActual, PageFit, PageWidth, and PageHeight with identical Description attributes. Consider reusing EnumZoomMode instead of creating a new enum to avoid code duplication and maintain consistency across the codebase.
Suggested change
| /// <summary> | |
| /// PdfReader 文档适配模式 | |
| /// </summary> | |
| public enum PdfReaderFitMode | |
| { | |
| /// <summary> | |
| /// 页面宽度 | |
| /// </summary> | |
| [Description("page-width")] | |
| PageWidth, | |
| /// <summary> | |
| /// 实际大小 | |
| /// </summary> | |
| [Description("page-actual")] | |
| PageActual, | |
| /// <summary> | |
| /// 页面高度 | |
| /// </summary> | |
| [Description("page-height")] | |
| PageHeight, | |
| /// <summary> | |
| /// 自适应宽高 | |
| /// </summary> | |
| [Description("page-fit")] | |
| PageFit, | |
| /// <summary> | |
| /// 自动 | |
| /// </summary> | |
| [Description("auto")] | |
| Auto | |
| } |
Comment on lines
+29
to
+33
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageActual)">page-actual</div> | ||
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageWidth)">page-width</div> | ||
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageHeight)">page-height</div> | ||
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageFit)">page-fit</div> | ||
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.Auto)">auto</div> |
There was a problem hiding this comment.
The dropdown items display raw technical values (e.g., "page-actual", "page-width") which are not user-friendly. Consider using localized or human-readable labels instead, similar to how the enum descriptions are used in Chinese comments (e.g., "实际大小" for PageActual, "页面宽度" for PageWidth).
Suggested change
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageActual)">page-actual</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageWidth)">page-width</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageHeight)">page-height</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageFit)">page-fit</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.Auto)">auto</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageActual)">实际大小</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageWidth)">页面宽度</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageHeight)">页面高度</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.PageFit)">适合页面</div> | |
| <div class="dropdown-item" @onclick="() => SetFitMode(PdfReaderFitMode.Auto)">自动</div> |
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 #728
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add configurable print visibility and enhanced zoom / fit controls to the PdfReader toolbar while updating fit mode handling and related UI/interop.
New Features:
Enhancements: