Conversation
Reviewer's GuideRefactors the PdfReader toolbar so that its behavior (scale, page navigation, fit modes, rotation, thumbnails, printing) is driven entirely from JavaScript via a new resetToolbarView helper and central event wiring, adds a public ResetToolbar JS interop hook, and removes unused PDF viewer dialog/sidebar CSS from the embedded pdf_viewer.css. Sequence diagram for the new PdfReader ResetToolbar JS interopsequenceDiagram
actor User
participant BlazorPdfReader as PdfReader_component
participant JSModule as PdfReader_razor_js
participant DOM as Toolbar_DOM
participant PdfViewer as PDFViewer
User->>BlazorPdfReader: Toggle_ShowToolbar_true
activate BlazorPdfReader
BlazorPdfReader->>BlazorPdfReader: OnAfterRenderAsync
BlazorPdfReader->>JSModule: resetToolbar(id)
deactivate BlazorPdfReader
activate JSModule
JSModule->>DOM: resetToolbarView(el,pdfViewer)
activate DOM
DOM-->>JSModule: toolbar_elements
JSModule->>DOM: updateScaleValue(el,pdfViewer.currentScale)
JSModule->>DOM: set_page_input(pdfViewer.currentPageNumber)
JSModule->>DOM: update_fit_height_class(pdfViewer.currentScaleValue)
JSModule->>DOM: update_twoPagesOneView_active(pdfViewer.spreadMode)
JSModule->>DOM: delete el.widths
JSModule->>DOM: relayoutToolbar(el)
DOM-->>JSModule: toolbar_layout_adjusted
JSModule-->>BlazorPdfReader: toolbar_state_synced
deactivate JSModule
User->>DOM: Click_toolbar_buttons
DOM->>JSModule: toolbar_events(click,change,focus)
JSModule->>PdfViewer: update_properties(scale,page,rotation,spreadMode)
PdfViewer-->>JSModule: eventBus_events(scalechanging,rotationchanging,pagechanging)
JSModule->>DOM: update_controls(scale_input,page_input,thumbnails)
Sequence diagram for centralized toolbar event handling in JSsequenceDiagram
actor User
participant DOM as Toolbar_DOM
participant JSModule as PdfReader_razor_js
participant PdfViewer as PDFViewer
participant EventBus as PDFJS_EventBus
User->>DOM: Click_bb_page_plus
DOM->>JSModule: toolbar_click_event
activate JSModule
JSModule->>JSModule: updateScale(pdfViewer,button,+1)
JSModule->>PdfViewer: set currentScale
deactivate JSModule
PdfViewer->>EventBus: fire scalechanging
EventBus->>JSModule: scalechanging(evt.scale)
JSModule->>DOM: updateScaleValue(el,evt.scale)
User->>DOM: Change_bb_view_num
DOM->>JSModule: toolbar_change_event
activate JSModule
JSModule->>PdfViewer: set currentPageNumber
deactivate JSModule
PdfViewer->>EventBus: fire pagechanging
EventBus->>JSModule: pagechanging(evt)
JSModule->>BlazorPdfReader: invokeMethodAsync PageChanged(page)
User->>DOM: Click_bb_view_fit_height
DOM->>JSModule: toolbar_click_event
activate JSModule
JSModule->>DOM: add_fit_height_class
JSModule->>PdfViewer: set currentScaleValue page_height
deactivate JSModule
Updated class diagram for PdfReader component public APIclassDiagram
class PdfReader {
+string Id
+string? Url
+PdfReaderFitMode FitMode
+bool EnableThumbnails
+bool ShowToolbar
+uint CurrentPage
+string? CurrentScale
+EventCallback OnDownloadAsync
+Task NavigateToPageAsync(uint pageNumber)
+Task SetFitMode(PdfReaderFitMode mode)
+Task RotateLeft()
+Task RotateRight()
+Task OnPagesInitAsync(int pagesCount)
+Task OnPagesLoadedAsync(int pagesCount)
+Task OnPageChangedAsync(uint pageNumber)
-string? _docTitle
-uint _currentPage
-string? _url
-bool _enableThumbnails
-bool _showToolbar
-Task OnAfterRenderAsync(bool firstRender)
-Task InvokeInitAsync()
}
class PdfReaderFitMode {
PageWidth
PageHeight
PageActual
}
PdfReader --> PdfReaderFitMode : uses
class PdfReaderJsModule {
+setScaleValue(id,value)
+rotate(id,step)
+navigateToPage(id,pageNumber)
+scale(id,scale)
+resetToolbar(id)
+resetThumbnails(id)
+dispose(id)
-rotateView(pdfViewer,step)
-setObserver(el)
-relayoutToolbar(el)
-addEventBus(el,pdfViewer,eventBus,invoke,options)
-addToolbarEventHandlers(el,pdfViewer,invoke,options)
-resetToolbarView(el,pdfViewer)
-resetThumbnailsView(el,pdfViewer)
-updateScaleValue(el,value)
-updateScale(pdfViewer,button,rate)
-printPdf(url)
}
PdfReader --> PdfReaderJsModule : JS_interop_calls
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 - here's some feedback:
- In
printPdfthe removal of theafterprinthandler means the print iframe is no longer cleaned up after each print and will only be removed on component disposal; consider restoring a per-print cleanup to avoid accumulating hidden iframes during long-lived sessions. - Several of the new toolbar helpers (e.g.
resetToolbarView,updateScaleValue,relayoutToolbar) assume that elements like.bb-view-scale-input,.bb-page-minus,.dropdown-item-pages, and.bb-view-group-rotateare always present; adding null checks will make this more robust against template changes or partial toolbars.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `printPdf` the removal of the `afterprint` handler means the print iframe is no longer cleaned up after each print and will only be removed on component disposal; consider restoring a per-print cleanup to avoid accumulating hidden iframes during long-lived sessions.
- Several of the new toolbar helpers (e.g. `resetToolbarView`, `updateScaleValue`, `relayoutToolbar`) assume that elements like `.bb-view-scale-input`, `.bb-page-minus`, `.dropdown-item-pages`, and `.bb-view-group-rotate` are always present; adding null checks will make this more robust against template changes or partial toolbars.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:216` </location>
<code_context>
+ addToolbarEventHandlers(el, pdfViewer, invoke, options);
})
eventBus.on("pagechanging", async evt => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The JS page change event no longer updates the toolbar page input, so UI state can diverge from the viewer state.
After removing `@bind` from the page input, `pagechanging` now only updates thumbnails and calls the .NET callback; it doesn’t update `.bb-view-num`. Since navigation can happen via thumbnails, keyboard, or programmatically, the textbox can fall out of sync with the actual page. Please also set `el.querySelector('.bb-view-num')?.value = String(page);` (with appropriate null checks) here so the toolbar page indicator always reflects the current page.
Suggested implementation:
```javascript
eventBus.on("pagechanging", async evt => {
}
const page = evt?.pageNumber ?? pdfViewer.currentPageNumber;
const pageInput = el.querySelector(".bb-view-num");
if (pageInput) {
pageInput.value = String(page);
}
if (options.triggerPageChanged === true) {
await invoke.invokeMethodAsync("PageChanged", page);
}
}, true);
```
1. If `evt.pageNumber` is not the correct property name for the PDF.js event in your version, adjust `evt?.pageNumber` to the appropriate field (e.g. `evt.pageNumber`, `evt.pageNumber`, or `evt.pageNumber || pdfViewer.currentPageNumber` depending on your actual event shape).
2. Ensure no other code inside the `pagechanging` handler relies on a pre-existing `page` variable name; if it does, you can reuse that variable instead of redeclaring `const page = ...` to avoid shadowing.
3. If your codebase already has a helper for updating toolbar controls (e.g. centralizing `.bb-view-num` updates), consider calling that helper here instead of directly querying and assigning the value to keep logic consistent.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:473` </location>
<code_context>
const printPdf = url => {
</code_context>
<issue_to_address>
**issue (performance):** Removing the `afterprint` handler means print iframes are never cleaned up from the DOM.
The old `afterprint` handler removed the temporary iframe after each print; without any cleanup, every print leaves another iframe in the DOM, which can accumulate in long‑lived sessions. If `afterprint` is problematic in some browsers, consider an alternative cleanup mechanism (e.g., delayed removal in `onload` or another suitable event) rather than never removing the iframe.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:116` </location>
<code_context>
+ return observer;
+}
+
+const relayoutToolbar = el => {
+ const toolbar = el.querySelector(".bb-view-toolbar");
+ if (toolbar === null) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the toolbar layout, event wiring, and scale handling into cached, data-driven helpers to reduce duplication and make the viewer logic easier to follow and maintain.
The new functionality is solid, but some of the complexity the other reviewer called out can be reduced without changing behavior. A few targeted extractions and data structures will make this easier to maintain.
### 1. Toolbar relayout: make it data-driven and cache elements
`relayoutToolbar` currently re-queries the DOM and hard-codes the hide/show order twice. You can simplify this by:
* Caching toolbar elements once (on `el`) so `relayoutToolbar` doesn’t query every time.
* Representing the hide/show order as an array and looping over it instead of chained `if/else if`.
Example:
```js
// called once after toolbar is rendered
const initToolbarLayoutState = (el) => {
const toolbar = el.querySelector(".bb-view-toolbar");
if (!toolbar) return;
const title = el.querySelector(".bb-view-title");
const subject = el.querySelector(".bb-view-subject");
const groupPage = el.querySelector(".bb-view-group-page");
const groupScale = el.querySelector(".bb-view-group-scale");
const groupRotate = el.querySelector(".bb-view-group-rotate");
const controls = el.querySelector(".bb-view-controls");
el.toolbarLayout = {
toolbar,
title,
items: [
{ el: subject, widthIndex: 0 },
{ el: groupRotate, widthIndex: 1 },
{ el: groupScale, widthIndex: 2 },
{ el: groupPage, widthIndex: 3 },
{ el: controls, widthIndex: 4 }
],
widths: undefined,
};
};
```
Then `relayoutToolbar` becomes:
```js
const relayoutToolbar = (el) => {
const state = el.toolbarLayout;
if (!state || !state.toolbar || !state.title) return;
const { toolbar, title, items } = state;
if (!state.widths) {
state.widths = items.map(i => i.el?.offsetWidth || 0);
}
const getActualWidth = () =>
title.offsetWidth +
items.reduce((acc, i) => acc + (i.el?.offsetWidth || 0), 0);
// shrink
while (getActualWidth() + 8 > toolbar.offsetWidth) {
const itemToHide = items.find(i => i.el && !i.el.classList.contains("d-none"));
if (!itemToHide) break;
itemToHide.el.classList.add("d-none");
}
// grow (respecting original order)
for (const item of [...items].reverse()) {
if (!item.el || !item.el.classList.contains("d-none")) continue;
const needed = state.widths[item.widthIndex];
if (getActualWidth() + needed < toolbar.offsetWidth) {
item.el.classList.remove("d-none");
}
}
};
```
`setObserver` can then just ensure `initToolbarLayoutState` is called once:
```js
const setObserver = (el) => {
initToolbarLayoutState(el);
const observer = new ResizeObserver(() => relayoutToolbar(el));
observer.observe(el);
return observer;
};
```
This removes duplicated ordering logic and repeated `querySelector` calls.
### 2. Toolbar event wiring: centralize and extract tiny handlers
`addToolbarEventHandlers` is doing a lot in inline callbacks. You can keep the behavior but move logic into small helpers and pass them to the event wiring. This makes it easier to see “what the toolbar does” at a glance.
For example:
```js
const handlePageNumberChange = (pdfViewer, e) => {
let pageNumber = parseInt(e.delegateTarget.value) || 1;
if (pageNumber < 1) pageNumber = 1;
if (pageNumber > pdfViewer.pagesCount) pageNumber = pdfViewer.pagesCount;
pdfViewer.currentPageNumber = pageNumber;
};
const handleFitWidth = (el, pdfViewer) => {
const group = el.querySelector('.bb-view-group-rotate');
group?.classList.remove('fit-height');
pdfViewer.currentScaleValue = 'page-width';
};
```
Then `addToolbarEventHandlers` becomes more declarative:
```js
const addToolbarEventHandlers = (el, pdfViewer, invoke, options) => {
const toolbar = el.querySelector(".bb-view-toolbar");
if (!toolbar) return;
EventHandler.on(toolbar, "click", ".bb-view-bar", () =>
toggleThumbnails(el)
);
EventHandler.on(toolbar, "change", ".bb-view-num", e =>
handlePageNumberChange(pdfViewer, e)
);
EventHandler.on(toolbar, "click", ".bb-page-minus", e =>
updateScale(pdfViewer, e.delegateTarget, -1)
);
EventHandler.on(toolbar, "click", ".bb-page-plus", e =>
updateScale(pdfViewer, e.delegateTarget, 1)
);
EventHandler.on(toolbar, "click", ".bb-view-fit-width", () =>
handleFitWidth(el, pdfViewer)
);
// ... similar for fit-height, page-actual, rotate-left/right, print, spread toggle, etc.
};
```
You already centralized teardown with `EventHandler.off(toolbar, "click/change/focus")`; extracting handlers further reduces the cognitive load while keeping that disposal simple.
You could also wrap this with a single entry point so `addEventBus` stays focused on viewer events:
```js
const setupToolbar = (el, pdfViewer, invoke, options, eventBus) => {
initToolbarLayoutState(el);
addToolbarEventHandlers(el, pdfViewer, invoke, options);
eventBus.on("scalechanging", evt => updateScaleValue(el, evt.scale));
eventBus.on("rotationchanging", evt => updateThumbnailRotation(el, evt.pagesRotation));
};
```
Then from `pagesloaded`:
```js
eventBus.on("pagesloaded", async e => {
// existing logic ...
setupToolbar(el, pdfViewer, invoke, options, eventBus);
});
```
### 3. Scale handling: cache controls instead of querying each time
`updateScaleValue` queries `.bb-page-minus`, `.bb-page-plus`, and `.bb-view-scale-input` on every call. These elements are stable; you can resolve once and store them.
```js
const initScaleControls = (el) => {
const minus = el.querySelector(".bb-page-minus");
const plus = el.querySelector(".bb-page-plus");
const scaleEl = el.querySelector(".bb-view-scale-input");
el.scaleControls = { minus, plus, scaleEl };
};
const updateScaleValue = (el, value) => {
const controls = el.scaleControls || {};
const { minus, plus, scaleEl } = controls;
if (!scaleEl || !minus || !plus) return;
const scale = value * 100;
scaleEl.value = `${Math.round(scale, 0)}%`;
if (scale === 25) {
minus.classList.add("disabled");
plus.classList.remove("disabled");
} else if (scale === 500) {
plus.classList.add("disabled");
minus.classList.remove("disabled");
} else {
minus.classList.remove("disabled");
plus.classList.remove("disabled");
}
};
```
Call `initScaleControls(el)` once (e.g., inside `setupToolbar` or `addToolbarEventHandlers`).
You can also pull the step logic out of `updateScale`:
```js
const SCALE_STEPS = [25, 33, 50, 67, 75, 80, 90, 100, 110, 125, 150, 175, 200, 250, 300, 400, 500];
const getNextScale = (current, rate) => {
const candidates = SCALE_STEPS.filter(s => rate > 0 ? current < s : current > s);
if (candidates.length === 0) return null;
return rate > 0 ? candidates[0] : candidates[candidates.length - 1];
};
const updateScale = (pdfViewer, button, rate) => {
if (button.classList.contains("disabled")) return;
const current = Math.round(pdfViewer.currentScale * 100, 0);
const next = getNextScale(current, rate);
if (next == null) return;
pdfViewer.currentScaleValue = next / 100;
};
```
This keeps the branching tiny and self-documenting.
### 4. Toolbar reset: split by concern
`resetToolbarView` currently mixes several responsibilities (scale, page, layout mode, layout cache). Splitting it into focused helpers will keep behavior but make maintenance easier:
```js
const resetScaleControls = (el, pdfViewer) => {
updateScaleValue(el, pdfViewer.currentScale);
};
const resetPageControls = (el, pdfViewer) => {
const pageEl = el.querySelector(".bb-view-num");
if (pageEl) {
pageEl.value = pdfViewer.currentPageNumber;
}
};
const resetLayoutMode = (el, pdfViewer) => {
const group = el.querySelector(".bb-view-group-rotate");
if (group) {
group.classList.toggle("fit-height", pdfViewer.currentScaleValue === "page-height");
}
const twoPagesOneView = el.querySelector(".dropdown-item-pages");
if (twoPagesOneView) {
twoPagesOneView.classList.toggle("active", pdfViewer.spreadMode === 1);
}
};
const resetToolbarLayoutCache = (el) => {
delete el.widths;
relayoutToolbar(el);
};
const resetToolbarView = (el, pdfViewer) => {
resetScaleControls(el, pdfViewer);
resetPageControls(el, pdfViewer);
resetLayoutMode(el, pdfViewer);
resetToolbarLayoutCache(el);
};
```
This preserves the existing behavior but makes the side effects obvious at the call site.
---
All of these are small, local refactors that don’t change functionality but:
* Avoid repeated DOM queries.
* Turn hard-coded `if/else` chains into data-driven loops.
* Separate responsibilities into small, named helpers.
* Keep disposal logic as simple as it is now.
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:176-180` </location>
<code_context>
else if (subject.classList.contains("d-none")) {
if (getActualWidth() + el.widths[0] < toolbar.offsetWidth) {
subject.classList.remove("d-none");
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
else if (subject.classList.contains("d-none") && getActualWidth() + el.widths[0] < toolbar.offsetWidth) {
subject.classList.remove("d-none");
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 5
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:260-262` </location>
<code_context>
if (pageNumber < 1) {
pageNumber = 1;
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use max/min instead of if statement ([`max-min-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/max-min-identity))
```suggestion
pageNumber = Math.max(pageNumber, 1)
```
<br/><details><summary>Explanation</summary>We often need to work out the smallest or largest of two values, and the
most readable way to do this is to use the built-in `min` and `max`
functions. This results in a shorter and clearer way to achieve the same result.
</details>
</issue_to_address>
### Comment 6
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:263-265` </location>
<code_context>
if (pageNumber > pdfViewer.pagesCount) {
pageNumber = pdfViewer.pagesCount;
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use max/min instead of if statement ([`max-min-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/max-min-identity))
```suggestion
pageNumber = Math.min(pageNumber, pdfViewer.pagesCount);
```
<br/><details><summary>Explanation</summary>We often need to work out the smallest or largest of two values, and the
most readable way to do this is to use the built-in `min` and `max`
functions. This results in a shorter and clearer way to achieve the same result.
</details>
</issue_to_address>
### Comment 7
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:290-292` </location>
<code_context>
else if (value > 500) {
value = 500;
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use max/min instead of if statement ([`max-min-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/max-min-identity))
```suggestion
else value = Math.min(value, 500);
```
<br/><details><summary>Explanation</summary>We often need to work out the smallest or largest of two values, and the
most readable way to do this is to use the built-in `min` and `max`
functions. This results in a shorter and clearer way to achieve the same result.
</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.
There was a problem hiding this comment.
Pull request overview
This PR adds a ResetToolbarView function to the PdfReader component, addressing issue #748. The changes refactor how toolbar state is managed by moving event handling from inline Blazor events to JavaScript-based delegation, removing two-way binding for toolbar controls, and introducing a new reset mechanism when the toolbar visibility changes.
Key Changes
- Refactored toolbar event handling from Blazor
@onclickdirectives to JavaScript event delegation viaaddToolbarEventHandlers - Replaced two-way binding (
@bind) for page number and scale inputs with static values, now managed entirely in JavaScript - Added
resetToolbarJavaScript function andresetToolbarViewhelper to restore toolbar state whenShowToolbarchanges from false to true
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReader.razor.js | Added resetToolbar export, rotateView helper, and resetToolbarView function; refactored toolbar layout logic; moved all toolbar event handlers to addToolbarEventHandlers; updated disposal cleanup |
| PdfReader.razor | Moved toolbar wrapper div outside conditional rendering; removed @onclick and @bind directives in favor of JavaScript handling; simplified toolbar controls to use static initial values |
| PdfReader.razor.cs | Removed unused System.Globalization import; removed CurrentPageString, CurrentScaleString, ViewBodyString properties and related helper methods; changed SetFitMode from void to Task; added resetToolbar invocation logic; added CurrentPage to initialization options |
| PdfReader.razor.css | Updated CSS selectors from .bb-view-toolbar-main.fit-width to .bb-view-toolbar-main .fit-height to match new DOM structure |
| pdf_viewer.css | Removed 514 lines of unused dialog, sidebar, and color picker styles |
| EnumZoomMode.cs | Deleted entire file (unused enum) |
| EnumPageMode.cs | Deleted entire file (unused enum) |
| BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.4 to 10.0.6 |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:355
- Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
delete el.widths
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| minus.classList.add("disabled"); | ||
| EventHandler.on(toolbar, "click", '.bb-view-bar', e => { | ||
| const thumbnailsEl = el.querySelector(".bb-view-thumbnails"); | ||
| thumbnailsEl.classList.toggle("show"); |
There was a problem hiding this comment.
Potential null reference error: thumbnailsEl could be null if EnableThumbnails is false. Add a null check before accessing .classList.toggle().
Suggested change
| thumbnailsEl.classList.toggle("show"); | |
| if (thumbnailsEl) { | |
| thumbnailsEl.classList.toggle("show"); | |
| } |
| }); | ||
|
|
||
| iframe.contentWindow.focus(); | ||
| iframe.contentWindow.print(); |
There was a problem hiding this comment.
Memory leak potential: The removal of the 'afterprint' event listener cleanup means the iframe is no longer automatically removed after printing. This could cause memory leaks if users print multiple times. Consider restoring the cleanup logic or implementing an alternative cleanup mechanism.
Suggested change
| iframe.contentWindow.print(); | |
| iframe.contentWindow.print(); | |
| // Cleanup after printing | |
| const cleanup = () => { | |
| if (iframe && iframe.parentNode) { | |
| iframe.parentNode.removeChild(iframe); | |
| } | |
| window.removeEventListener('afterprint', cleanup); | |
| }; | |
| window.addEventListener('afterprint', cleanup); |
| const addEventBus = (el, pdfViewer, eventBus, invoke, options) => { | ||
| eventBus.on("pagesinit", async () => { | ||
| if (options.fitMode) { | ||
| pdfViewer.currentScaleValue = fitMode; |
There was a problem hiding this comment.
The variable fitMode is undefined. This should be options.fitMode to access the fitMode property from the options parameter.
Suggested change
| pdfViewer.currentScaleValue = fitMode; | |
| pdfViewer.currentScaleValue = options.fitMode; |
| if (thumbnailsContainer) { | ||
| thumbnailsContainer.style.setProperty('--thumb-rotate', `${evt.pagesRotation}deg`); | ||
| const pageEl = el.querySelector(".bb-view-num"); | ||
| pageEl.value = pdfViewer.currentPageNumber; |
There was a problem hiding this comment.
Potential null reference error: pageEl could be null if the toolbar is hidden (ShowToolbar=false). Add a null check before accessing .value, similar to how it's checked for group at line 338.
Suggested change
| pageEl.value = pdfViewer.currentPageNumber; | |
| if (pageEl) { | |
| pageEl.value = pdfViewer.currentPageNumber; | |
| } |
Comment on lines
+348
to
+353
| if (pdfViewer.spreadMode === 1) { | ||
| twoPagesOneView.classList.add("active"); | ||
| } | ||
| else { | ||
| twoPagesOneView.classList.remove("active"); | ||
| } |
There was a problem hiding this comment.
Potential null reference error: twoPagesOneView could be null if ShowTwoPagesOneView is false or the toolbar is hidden. Add a null check before accessing .classList.
Suggested change
| if (pdfViewer.spreadMode === 1) { | |
| twoPagesOneView.classList.add("active"); | |
| } | |
| else { | |
| twoPagesOneView.classList.remove("active"); | |
| } | |
| if (twoPagesOneView) { | |
| if (pdfViewer.spreadMode === 1) { | |
| twoPagesOneView.classList.add("active"); | |
| } | |
| else { | |
| twoPagesOneView.classList.remove("active"); | |
| } | |
| } |
| } | ||
| else if (controls.classList.contains('d-none') === false) { | ||
| controls.classList.add("d-none"); | ||
| } |
There was a problem hiding this comment.
Potential infinite loop: The while loop has no guaranteed exit condition. If all elements already have the 'd-none' class, the loop will continue indefinitely because getActualWidth() won't change. Add a break statement at the end of the if-else chain (after line 152) to exit when no more elements can be hidden.
Suggested change
| } | |
| } | |
| else { | |
| break; | |
| } |
Comment on lines
+272
to
+282
| group.classList.remove('fit-height') | ||
| pdfViewer.currentScaleValue = 'page-width'; | ||
| }); | ||
| EventHandler.on(toolbar, 'click', '.bb-view-fit-height', e => { | ||
| const group = el.querySelector('.bb-view-group-rotate'); | ||
| group.classList.add('fit-height') | ||
| pdfViewer.currentScaleValue = 'page-height'; | ||
| }); | ||
| EventHandler.on(toolbar, 'click', '.bb-view-page-actual', e => { | ||
| const group = el.querySelector('.bb-view-group-rotate'); | ||
| group.classList.remove('fit-height') |
There was a problem hiding this comment.
Potential null reference error: group could be null if the toolbar is hidden. Add a null check before accessing .classList on lines 272, 277, and 282, similar to how it's checked in resetToolbarView at line 338.
Suggested change
| group.classList.remove('fit-height') | |
| pdfViewer.currentScaleValue = 'page-width'; | |
| }); | |
| EventHandler.on(toolbar, 'click', '.bb-view-fit-height', e => { | |
| const group = el.querySelector('.bb-view-group-rotate'); | |
| group.classList.add('fit-height') | |
| pdfViewer.currentScaleValue = 'page-height'; | |
| }); | |
| EventHandler.on(toolbar, 'click', '.bb-view-page-actual', e => { | |
| const group = el.querySelector('.bb-view-group-rotate'); | |
| group.classList.remove('fit-height') | |
| if (group) group.classList.remove('fit-height'); | |
| pdfViewer.currentScaleValue = 'page-width'; | |
| }); | |
| EventHandler.on(toolbar, 'click', '.bb-view-fit-height', e => { | |
| const group = el.querySelector('.bb-view-group-rotate'); | |
| if (group) group.classList.add('fit-height'); | |
| pdfViewer.currentScaleValue = 'page-height'; | |
| }); | |
| EventHandler.on(toolbar, 'click', '.bb-view-page-actual', e => { | |
| const group = el.querySelector('.bb-view-group-rotate'); | |
| if (group) group.classList.remove('fit-height'); |
| }); | ||
| } | ||
| const resetToolbarView = (el, pdfViewer) => { | ||
| const scaleEl = el.querySelector(".bb-view-scale-input"); |
There was a problem hiding this comment.
Unused variable scaleEl.
Suggested change
| const scaleEl = el.querySelector(".bb-view-scale-input"); |
| return; | ||
| } | ||
|
|
||
| let v = 100; |
There was a problem hiding this comment.
The initial value of v is unused, since it is always overwritten.
Suggested change
| let v = 100; | |
| let v; |
| EventHandler.on(toolbar, "click", ".bb-view-print", async e => { | ||
| printPdf(options.url); | ||
| await invoke.invokeMethodAsync("Printing"); | ||
| }) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (93% of all statements in the enclosing function have an explicit semicolon).
Suggested change
| }) | |
| }); |
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 #748
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a client-side toolbar reset mechanism for the PdfReader and move most toolbar behavior from Blazor event handlers into JavaScript, while simplifying the embedded PDF viewer styles.
New Features:
Bug Fixes:
Enhancements: