feat(PdfReader): add width observer function#743
Merged
Conversation
Reviewer's GuideRefactors PdfReader initialization to separate PDF loading from layout logic and introduces a ResizeObserver-based toolbar width observer that dynamically hides/shows toolbar groups based on available space, along with corresponding Razor markup and CSS updates to support grouped toolbar layout and cleanup on dispose. Sequence diagram for PdfReader initialization and disposal with width observersequenceDiagram
participant Blazor as BlazorComponent
participant PdfModule as PdfReader_js
participant loadPdf as loadPdf
participant setObserver as setObserver
participant ResizeObs as ResizeObserver
participant Data as DataStore
Blazor->>PdfModule: init(id, invoke, options)
PdfModule->>loadPdf: loadPdf(el, invoke, options)
loadPdf->>loadPdf: pdfjsLib.getDocument(options)
loadPdf->>loadPdf: configure onProgress
loadPdf->>loadPdf: configure onPassword
loadPdf->>loadPdf: create EventBus
loadPdf->>loadPdf: create PDFViewer
loadPdf->>loadPdf: addEventListener(el, pdfViewer, eventBus, invoke, options)
loadPdf->>loadPdf: await loadingTask.promise
loadPdf->>loadPdf: pdfViewer.setDocument(pdfDocument)
loadPdf-->>PdfModule: pdfViewer
PdfModule->>setObserver: setObserver(el)
setObserver->>setObserver: query toolbar and groups
setObserver->>setObserver: compute initial widths
setObserver->>ResizeObs: new ResizeObserver(callback)
ResizeObs->>ResizeObs: observe(el)
setObserver-->>PdfModule: observer
PdfModule->>Data: Data.set(id, { el, pdfViewer, observer })
Note over ResizeObs: On resize
ResizeObs->>PdfModule: callback(entries)
PdfModule->>PdfModule: query .bb-view-toolbar
PdfModule->>PdfModule: decreaseSpace(toolbar)
PdfModule->>PdfModule: increaseSpace(toolbar)
Blazor->>PdfModule: dispose(id)
PdfModule->>Data: Data.get(id)
PdfModule->>ResizeObs: observer.disconnect()
PdfModule->>Data: Data.remove(id)
PdfModule->>PdfModule: cleanup DOM event handlers
Class diagram for updated PdfReader Razor component layout propertiesclassDiagram
class PdfReader {
+PdfReaderFitMode FitMode
-string Id
-string? ClassString
-string? StyleString
-string? ViewBodyString
+void SetFitMode(PdfReaderFitMode fitMode)
+void RotateRight()
+void OnDownload()
}
class PdfReaderFitMode {
<<enumeration>>
PageActual
PageWidth
PageHeight
}
PdfReader --> PdfReaderFitMode : uses
note for PdfReader "ViewBodyString now builds CSS classes 'bb-view-group bb-view-toolbar-main' instead of 'bb-view-body', aligning with grouped toolbar layout"
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
dispose, you're destructuringconst { el, observer } = Data.get(id);and then attempting to reassignobserver, which will throw in strict mode; instead, either avoid reassigning or use a local mutable variable, and consider falling back todocument.getElementById(id)ifData.get(id)returns nothing to preserve previous behavior. - The
setObserverfunction assumes all queried elements (.bb-view-title,.bb-view-subject,.bb-view-group-*,.bb-view-controls) are always present and non-null; this can break when certain toolbar areas are conditionally hidden (e.g.,ShowToolbaris false or individual features disabled), so add null checks or short-circuit early when any of the required elements are missing. - Consider guarding the
ResizeObserverusage insetObserverwith a feature check (e.g.,if (typeof ResizeObserver === 'undefined')) or a graceful fallback to avoid runtime errors in environments/browsers that do not support it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `dispose`, you're destructuring `const { el, observer } = Data.get(id);` and then attempting to reassign `observer`, which will throw in strict mode; instead, either avoid reassigning or use a local mutable variable, and consider falling back to `document.getElementById(id)` if `Data.get(id)` returns nothing to preserve previous behavior.
- The `setObserver` function assumes all queried elements (`.bb-view-title`, `.bb-view-subject`, `.bb-view-group-*`, `.bb-view-controls`) are always present and non-null; this can break when certain toolbar areas are conditionally hidden (e.g., `ShowToolbar` is false or individual features disabled), so add null checks or short-circuit early when any of the required elements are missing.
- Consider guarding the `ResizeObserver` usage in `setObserver` with a feature check (e.g., `if (typeof ResizeObserver === 'undefined')`) or a graceful fallback to avoid runtime errors in environments/browsers that do not support it.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:384-389` </location>
<code_context>
}
export function dispose(id) {
+ const { el, observer } = Data.get(id);
Data.remove(id);
- const el = document.getElementById(id);
+ if (observer) {
+ observer.disconnect();
+ observer = null;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reassigning `observer` after destructuring as `const` will throw at runtime.
In `dispose`, `observer` is created via `const { el, observer } = Data.get(id);` and then reassigned (`observer = null;`), which will raise a `TypeError` in strict mode. If you don’t actually need to reuse `observer`, just drop the reassignment and only call `observer.disconnect()` when it exists:
```js
export function dispose(id) {
const data = Data.get(id);
if (!data) return;
const { el, observer } = data;
Data.remove(id);
if (observer) {
observer.disconnect();
}
// ...rest of cleanup
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:96` </location>
<code_context>
+ return pdfViewer;
+}
+
+const setObserver = el => {
+ const title = el.querySelector(".bb-view-title");
+ const subject = el.querySelector(".bb-view-subject");
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `setObserver` to use a data-driven visibility model and correcting the `dispose` observer handling to reduce branching and DOM state complexity.
You can simplify `setObserver` quite a bit by making the toolbar behavior data‑driven and avoiding ad‑hoc DOM state (`el.widths`), while also fixing the `observer` lifecycle in `dispose`.
### 1. Replace imperative `increaseSpace` / `decreaseSpace` chains with data‑driven logic
Both functions encode the same ordering twice. You can centralize the order and visibility logic in one array and a pair of small helpers:
```js
const setObserver = el => {
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");
const groups = [
{ el: subject },
{ el: groupRotate },
{ el: groupScale },
{ el: groupPage },
{ el: controls }
];
// You can recalc widths on demand instead of storing on `el`
const getTotalWidth = () =>
title.offsetWidth +
groups.reduce((sum, { el }) => sum + (el.classList.contains("d-none") ? 0 : el.offsetWidth), 0);
const collapseToFit = toolbarWidth => {
// hide from lowest priority (end) until it fits
for (let i = 0; i < groups.length && getTotalWidth() > toolbarWidth; i++) {
const { el } = groups[i];
if (!el.classList.contains("d-none")) {
el.classList.add("d-none");
}
}
};
const expandToUseSpace = toolbarWidth => {
// show from highest priority (end) while still fitting
for (let i = groups.length - 1; i >= 0; i--) {
const { el } = groups[i];
if (!el.classList.contains("d-none")) continue;
// optimistically show, measure, then revert if it doesn't fit
el.classList.remove("d-none");
if (getTotalWidth() > toolbarWidth) {
el.classList.add("d-none");
}
}
};
const observer = new ResizeObserver(() => {
const toolbar = el.querySelector(".bb-view-toolbar");
if (!toolbar) return;
const toolbarWidth = toolbar.offsetWidth;
collapseToFit(toolbarWidth);
expandToUseSpace(toolbarWidth);
});
observer.observe(el);
return observer;
};
```
Key improvements:
- The priority/order is encoded once in `groups`.
- Width sums are computed via `getTotalWidth()` instead of repeated expressions.
- No need for `el.widths` or mixing layout state into the DOM element.
- Visibility logic is declarative: “these groups in this order, hide until it fits, then show while it fits.”
You can tweak the collapse/expand order to exactly match your current behavior (e.g., start collapsing from `subject` first, etc.) by changing the order in `groups` or the iteration direction.
### 2. Avoid `el.widths` as ad‑hoc storage
The above refactor removes `el.widths` entirely. If you still want cached widths, keep them in a closure variable instead of mutating the DOM node:
```js
const widths = new Map(groups.map(g => [g.el, g.el.offsetWidth]));
// then use `widths.get(el)` instead of `el.widths[...]`
```
This keeps layout state local and self‑contained.
### 3. Fix `dispose` observer handling
You’re destructuring `observer` as a `const` and then trying to reassign it. Also, `Data.remove(id)` is called before you use `el`/`observer`. A safer pattern:
```js
export function dispose(id) {
const data = Data.get(id);
Data.remove(id);
if (!data) return;
const { el, observer } = data;
if (observer) {
observer.disconnect();
// no need to null it here, Data entry is already removed
}
if (el) {
const minus = el.querySelector(".bb-page-minus");
const plus = el.querySelector(".bb-page-plus");
if (minus) EventHandler.off(minus, "click");
if (plus) EventHandler.off(plus, "click");
}
}
```
This keeps the observer lifecycle clear and avoids illegal reassignment.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:127-131` </location>
<code_context>
else if (subject.classList.contains("d-none")) {
if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < 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") && title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return pdfViewer; | ||
| } | ||
|
|
||
| const setObserver = el => { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring setObserver to use a data-driven visibility model and correcting the dispose observer handling to reduce branching and DOM state complexity.
You can simplify setObserver quite a bit by making the toolbar behavior data‑driven and avoiding ad‑hoc DOM state (el.widths), while also fixing the observer lifecycle in dispose.
1. Replace imperative increaseSpace / decreaseSpace chains with data‑driven logic
Both functions encode the same ordering twice. You can centralize the order and visibility logic in one array and a pair of small helpers:
const setObserver = el => {
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");
const groups = [
{ el: subject },
{ el: groupRotate },
{ el: groupScale },
{ el: groupPage },
{ el: controls }
];
// You can recalc widths on demand instead of storing on `el`
const getTotalWidth = () =>
title.offsetWidth +
groups.reduce((sum, { el }) => sum + (el.classList.contains("d-none") ? 0 : el.offsetWidth), 0);
const collapseToFit = toolbarWidth => {
// hide from lowest priority (end) until it fits
for (let i = 0; i < groups.length && getTotalWidth() > toolbarWidth; i++) {
const { el } = groups[i];
if (!el.classList.contains("d-none")) {
el.classList.add("d-none");
}
}
};
const expandToUseSpace = toolbarWidth => {
// show from highest priority (end) while still fitting
for (let i = groups.length - 1; i >= 0; i--) {
const { el } = groups[i];
if (!el.classList.contains("d-none")) continue;
// optimistically show, measure, then revert if it doesn't fit
el.classList.remove("d-none");
if (getTotalWidth() > toolbarWidth) {
el.classList.add("d-none");
}
}
};
const observer = new ResizeObserver(() => {
const toolbar = el.querySelector(".bb-view-toolbar");
if (!toolbar) return;
const toolbarWidth = toolbar.offsetWidth;
collapseToFit(toolbarWidth);
expandToUseSpace(toolbarWidth);
});
observer.observe(el);
return observer;
};Key improvements:
- The priority/order is encoded once in
groups. - Width sums are computed via
getTotalWidth()instead of repeated expressions. - No need for
el.widthsor mixing layout state into the DOM element. - Visibility logic is declarative: “these groups in this order, hide until it fits, then show while it fits.”
You can tweak the collapse/expand order to exactly match your current behavior (e.g., start collapsing from subject first, etc.) by changing the order in groups or the iteration direction.
2. Avoid el.widths as ad‑hoc storage
The above refactor removes el.widths entirely. If you still want cached widths, keep them in a closure variable instead of mutating the DOM node:
const widths = new Map(groups.map(g => [g.el, g.el.offsetWidth]));
// then use `widths.get(el)` instead of `el.widths[...]`This keeps layout state local and self‑contained.
3. Fix dispose observer handling
You’re destructuring observer as a const and then trying to reassign it. Also, Data.remove(id) is called before you use el/observer. A safer pattern:
export function dispose(id) {
const data = Data.get(id);
Data.remove(id);
if (!data) return;
const { el, observer } = data;
if (observer) {
observer.disconnect();
// no need to null it here, Data entry is already removed
}
if (el) {
const minus = el.querySelector(".bb-page-minus");
const plus = el.querySelector(".bb-page-plus");
if (minus) EventHandler.off(minus, "click");
if (plus) EventHandler.off(plus, "click");
}
}This keeps the observer lifecycle clear and avoids illegal reassignment.
Comment on lines
+127
to
+131
| else if (subject.classList.contains("d-none")) { | ||
| if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | ||
| subject.classList.remove("d-none"); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| else if (subject.classList.contains("d-none")) { | |
| if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| subject.classList.remove("d-none"); | |
| } | |
| } | |
| else if (subject.classList.contains("d-none") && title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| subject.classList.remove("d-none"); | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
There was a problem hiding this comment.
Pull request overview
This PR adds responsive width observer functionality to the PdfReader component to dynamically hide/show toolbar elements based on available space. The implementation uses ResizeObserver to monitor container width changes and progressively hides toolbar groups (subject, rotate, scale, page controls) when space is constrained, then shows them again when space becomes available.
Key Changes:
- Extracted PDF loading logic into a separate
loadPdffunction for better code organization - Added
setObserverfunction that implements ResizeObserver-based responsive behavior for the toolbar - Restructured toolbar markup with grouped elements using
bb-view-groupclasses for better responsive control
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReader.razor.js | Refactored init function, added loadPdf and setObserver functions with ResizeObserver logic, updated dispose to cleanup observer |
| PdfReader.razor | Restructured toolbar HTML with grouped div elements (bb-view-group-*) to support responsive hiding |
| PdfReader.razor.css | Added bb-view-group class, renamed bb-view-body to bb-view-toolbar-main, removed duplicate flex properties |
| PdfReader.razor.cs | Updated ViewBodyString property to use new CSS class names |
| pdf_viewer.css | Removed unused loading icon CSS rules |
| BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.4-beta02 to 10.0.4-beta03 |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:389
- Assignment to variable observer, which is declared constant.
observer = null;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const el = document.getElementById(id); | ||
| if (observer) { | ||
| observer.disconnect(); | ||
| observer = null; |
There was a problem hiding this comment.
Assignment to observer has no effect as it's a function parameter. The assignment observer = null only modifies the local variable and doesn't prevent memory leaks. Since the observer is already disconnected on line 388, this line can be removed or is simply redundant.
Suggested change
| observer = null; |
| } | ||
| } | ||
| else if (groupScale.classList.contains('d-none')) { | ||
| if (title.offsetWidth + el.widths[2] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
There was a problem hiding this comment.
The width calculation is missing subject.offsetWidth and incorrectly includes the hidden element's current width. When checking if groupScale can be unhidden, the formula should be: title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + el.widths[2] + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth. The current calculation omits subject and uses groupScale.offsetWidth (which is 0 when hidden) instead of just using el.widths[2].
Suggested change
| if (title.offsetWidth + el.widths[2] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + el.widths[2] + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
| } | ||
| } | ||
| else if (groupRotate.classList.contains('d-none')) { | ||
| if (title.offsetWidth + el.widths[1] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
There was a problem hiding this comment.
The width calculation is missing subject.offsetWidth and incorrectly includes the hidden element's current width. When checking if groupRotate can be unhidden, the formula should be: title.offsetWidth + subject.offsetWidth + el.widths[1] + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth. The current calculation omits subject and uses groupRotate.offsetWidth (which is 0 when hidden) instead of just using el.widths[1].
Suggested change
| if (title.offsetWidth + el.widths[1] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + subject.offsetWidth + el.widths[1] + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
| } | ||
| } | ||
| const decreaseSpace = toolbar => { | ||
| while (title.offsetWidth + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth > toolbar.offsetWidth) { |
There was a problem hiding this comment.
The width calculation is missing subject.offsetWidth. The while condition should be: title.offsetWidth + subject.offsetWidth + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth > toolbar.offsetWidth. Without including subject's width when it's visible, the function will not hide elements when it should, causing toolbar overflow.
Suggested change
| while (title.offsetWidth + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth > toolbar.offsetWidth) { | |
| while ( | |
| title.offsetWidth | |
| + (subject.classList.contains("d-none") ? 0 : subject.offsetWidth) | |
| + groupPage.offsetWidth | |
| + groupScale.offsetWidth | |
| + groupRotate.offsetWidth | |
| + controls.offsetWidth | |
| > toolbar.offsetWidth | |
| ) { |
| const groupScale = el.querySelector(".bb-view-group-scale"); | ||
| const groupRotate = el.querySelector(".bb-view-group-rotate"); | ||
| const controls = el.querySelector(".bb-view-controls"); | ||
|
|
There was a problem hiding this comment.
Missing null checks for DOM elements. If ShowToolbar is false or any toolbar elements are not rendered, the querySelector calls (lines 97-102) will return null, causing line 104 to throw an error when accessing .offsetWidth on null. Add null checks before accessing offsetWidth properties or return early if toolbar elements are not found.
Suggested change
| // Null checks for all toolbar elements | |
| if (!title || !subject || !groupPage || !groupScale || !groupRotate || !controls) { | |
| return; | |
| } |
|
|
||
| const increaseSpace = toolbar => { | ||
| if (controls.classList.contains('d-none')) { | ||
| if (title.offsetWidth + el.widths[4] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
There was a problem hiding this comment.
The width calculation is missing subject.offsetWidth. When checking if controls can be unhidden, the formula should include all elements: title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + groupScale.offsetWidth + groupPage.offsetWidth + el.widths[4] < toolbar.offsetWidth. The current calculation omits subject, which could cause controls to be shown even when there isn't actually enough space.
Suggested change
| if (title.offsetWidth + el.widths[4] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + el.widths[0] + el.widths[4] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
| } | ||
| } | ||
| else if (groupPage.classList.contains('d-none')) { | ||
| if (title.offsetWidth + el.widths[3] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
There was a problem hiding this comment.
The width calculation is missing subject.offsetWidth and incorrectly includes the hidden element's current width. When checking if groupPage can be unhidden, the formula should be: title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + groupScale.offsetWidth + el.widths[3] + controls.offsetWidth < toolbar.offsetWidth. The current calculation omits subject and uses groupPage.offsetWidth (which is 0 when hidden) instead of just using el.widths[3].
Suggested change
| if (title.offsetWidth + el.widths[3] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + groupScale.offsetWidth + el.widths[3] + controls.offsetWidth < toolbar.offsetWidth) { |
| } | ||
| } | ||
| else if (subject.classList.contains("d-none")) { | ||
| if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
There was a problem hiding this comment.
The width calculation incorrectly includes both el.widths[0] and the sum of other current widths. When checking if subject can be unhidden, since all other elements are visible at this point, the formula should simply be: title.offsetWidth + el.widths[0] + groupRotate.offsetWidth + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth. Remove the redundant addition as it's already using stored width for subject.
Suggested change
| if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + el.widths[0] + groupRotate.offsetWidth + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
Comment on lines
+153
to
+158
| const observer = new ResizeObserver(entries => { | ||
| const toolbar = el.querySelector(".bb-view-toolbar"); | ||
| if (toolbar === null) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The ResizeObserver callback queries the DOM for .bb-view-toolbar on every resize event. Consider querying this element once during initialization (line 96-104) and storing it in a closure variable, then reuse it in the callback. This avoids repeated DOM queries on every resize.
Suggested change
| const observer = new ResizeObserver(entries => { | |
| const toolbar = el.querySelector(".bb-view-toolbar"); | |
| if (toolbar === null) { | |
| return; | |
| } | |
| const toolbar = el.querySelector(".bb-view-toolbar"); | |
| if (toolbar === null) { | |
| return; | |
| } | |
| const observer = new ResizeObserver(entries => { |
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 #742
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce responsive toolbar width handling to the PdfReader component and refactor PDF initialization logic.
New Features:
Enhancements: