Conversation
Reviewer's GuideImplements a configurable PDF viewer fit mode that reacts to parameter changes and localizes document properties labels in the PdfReader dialog, including initializing and tracking fit mode state across renders and updating locale resources. Sequence diagram for updating PDF fit mode on parameter changesequenceDiagram
actor User
participant ParentComponent
participant BlazorRenderer
participant PdfReader
participant JsRuntime
User->>ParentComponent: Interact to change fit mode
ParentComponent->>BlazorRenderer: Render PdfReader with new FitMode parameter
BlazorRenderer->>PdfReader: SetParametersAsync with FitMode
BlazorRenderer->>PdfReader: OnAfterRenderAsync(firstRender = false)
PdfReader->>PdfReader: Detect FitMode change (_fitMode != FitMode)
PdfReader->>PdfReader: Update _fitMode from FitMode
PdfReader->>PdfReader: SetFitMode(_fitMode)
PdfReader->>JsRuntime: InvokeVoidAsync setFitMode(id, _fitMode)
JsRuntime-->>PdfReader: Apply new fit mode in viewer
PdfReader-->>User: Updated PDF display according to new fit mode
Class diagram for PdfReader fit mode implementationclassDiagram
class PdfReader {
- string url
- string id
- bool enableThumbnails
- bool showToolbar
- PdfReaderFitMode fitMode
- string dropdownItemDefaultIcon
- int currentPage
+ PdfReader()
+ PdfReaderFitMode FitMode
+ bool EnableThumbnails
+ bool ShowToolbar
+ int CurrentPage
+ string Url
+ Task OnAfterRenderAsync(bool firstRender)
+ Task SetFitMode(PdfReaderFitMode fitMode)
}
class PdfReaderFitMode {
<<enumeration>>
PageActual
PageWidth
PageHeight
}
PdfReader --> PdfReaderFitMode : uses
File-Level Changes
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.
- Removing
PageFitandAutofromPdfReaderFitMode(and changing the enum shape) is a breaking change for consumers; consider either keeping the old values (even if unused) or introducing a non-breaking migration path. - In
OnAfterRenderAsync, the initialFitModeis stored duringfirstRenderbutSetFitModeis only called when_fitMode != FitMode, so the initial fit mode may never be applied; consider invokingSetFitModeon first render as well. - The dialog now localizes labels but keeps hardcoded values like "No" and "-"; consider localizing these values or using resource-based placeholders for consistency across languages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Removing `PageFit` and `Auto` from `PdfReaderFitMode` (and changing the enum shape) is a breaking change for consumers; consider either keeping the old values (even if unused) or introducing a non-breaking migration path.
- In `OnAfterRenderAsync`, the initial `FitMode` is stored during `firstRender` but `SetFitMode` is only called when `_fitMode != FitMode`, so the initial fit mode may never be applied; consider invoking `SetFitMode` on first render as well.
- The dialog now localizes labels but keeps hardcoded values like "No" and "-"; consider localizing these values or using resource-based placeholders for consistency across languages.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReaderFitMode.cs:12-16` </location>
<code_context>
/// </summary>
public enum PdfReaderFitMode
{
- /// <summary>
</code_context>
<issue_to_address>
**issue (bug_risk):** Enum reordering removes members and changes underlying values, which may break persisted or serialized values.
The original enum implicitly used values 0–4 for `PageWidth`, `PageActual`, `PageHeight`, `PageFit`, and `Auto`. The new version both removes `PageFit`/`Auto` and reorders the remaining members, so their underlying integer values change. If these values are stored (DB, URLs, local storage) or cross process boundaries (JS interop, config, etc.), this becomes a silent breaking change on upgrade.
To avoid that, keep the original numeric mapping with explicit values, and reserve the removed ones, e.g.:
```csharp
public enum PdfReaderFitMode
{
[Description("page-width")]
PageWidth = 0,
[Description("page-actual")]
PageActual = 1,
[Description("page-height")]
PageHeight = 2,
// PageFit = 3,
// Auto = 4,
}
```
This preserves compatibility while hiding the removed options from the public API.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor:167-168` </location>
<code_context>
</div>
<div class="bb-view-pdf-dialog-item">
- <div class="bb-view-pdf-dialog-label">Fast web view:</div>
+ <div class="bb-view-pdf-dialog-label">@Localizer["DocumentFastWebView"]</div>
<div class="bb-view-pdf-dialog-webview">No</div>
</div>
<div class="bb-view-pdf-dialog-close">
</code_context>
<issue_to_address>
**suggestion:** The 'No' literal for Fast Web View is not localized, unlike the surrounding labels.
The label is now localized via `Localizer[...]`, but the Fast Web View value is still hard-coded as `"No"`, which breaks localization consistency and affects non-English users.
Please localize this value as well (e.g. `@Localizer["DocumentFastWebViewDisabled"]` or a shared localized `Yes`/`No`), and ensure all possible values for this field are localized if it can be dynamic in the future.
Suggested implementation:
```
<div class="bb-view-pdf-dialog-item">
<div class="bb-view-pdf-dialog-label">@Localizer["DocumentFastWebView"]</div>
<div class="bb-view-pdf-dialog-webview">@Localizer["DocumentFastWebViewDisabled"]</div>
</div>
```
1. Add a `"DocumentFastWebViewDisabled"` entry to the relevant `.resx`/localization resources for all supported languages.
2. If this Fast Web View value becomes dynamic (e.g. Yes/No) in the future, ensure any other states (like `"DocumentFastWebViewEnabled"`) are also localized using the same `Localizer[...]` pattern.
</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 dynamic FitMode switching capability to the PdfReader component and improves localization of document properties. The main purpose is to enable runtime changes to the PDF viewer's fit mode (PageActual, PageWidth, PageHeight) by implementing change detection and a SetFitMode method. Additionally, it localizes hardcoded English strings in the document properties dialog.
Key Changes:
- Added dynamic FitMode parameter change detection in OnAfterRenderAsync to call SetFitMode when the property changes
- Reorganized PdfReaderFitMode enum by removing PageFit and Auto values and reordering remaining values
- Converted hardcoded English text in document properties dialog to use localized strings via @localizer syntax
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 |
|---|---|
| PdfReaderFitMode.cs | Simplified enum by removing PageFit and Auto values, reordered remaining values |
| PdfReader.razor.cs | Added _fitMode field and change detection logic to support dynamic FitMode updates |
| PdfReader.razor | Replaced hardcoded English strings with localized @localizer keys and added default "-" values |
| zh.json | Added Chinese translations for document properties fields |
| en.json | Added English translations for document properties fields |
| BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.7 to 10.0.8 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "DocumentVersion": "PDF 版本:", | ||
| "DocumentPageCount": "页数:", | ||
| "DocumentPageSize": "页面尺寸:", | ||
| "DocumentFastWebView": "Fast web view:" |
There was a problem hiding this comment.
The 'DocumentFastWebView' localization value in the Chinese (zh.json) file is not translated and remains in English. It should be translated to Chinese for consistency. Consider translating to something like '快速网页浏览:' or '快速 Web 视图:' to match the localization pattern used for other fields.
Suggested change
| "DocumentFastWebView": "Fast web view:" | |
| "DocumentFastWebView": "快速网页浏览:" |
| "DocumentProperty": "Document properties", | ||
| "CloseButtonText": "Close" | ||
| "CloseButtonText": "Close", | ||
| "DocumentProperties": "Document properties", |
There was a problem hiding this comment.
The key 'DocumentProperties' has the same value as the existing 'DocumentProperty' key (line 15). This creates duplicate localization entries with identical values. Consider removing the duplicate 'DocumentProperties' entry and using 'DocumentProperty' consistently, or clarify if they serve different purposes.
Suggested change
| "DocumentProperties": "Document properties", |
| "DocumentProperty": "文档属性", | ||
| "CloseButtonText": "关闭" | ||
| "CloseButtonText": "关闭", | ||
| "DocumentProperties": "文档属性", |
There was a problem hiding this comment.
The key 'DocumentProperties' has the same value as the existing 'DocumentProperty' key (line 15). This creates duplicate localization entries with identical values. Consider removing the duplicate 'DocumentProperties' entry and using 'DocumentProperty' consistently, or clarify if they serve different purposes.
Suggested change
| "DocumentProperties": "文档属性", |
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 #753
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge