feat(PdfReader): add thumbnails rotate function#725
Merged
Conversation
Reviewer's GuideAdds rotation-aware thumbnail support to the PdfReader by refactoring thumbnail initialization into a helper and wiring page rotation events to a new CSS-driven transform for thumbnail items. Sequence diagram for PdfReader thumbnail rotation handlingsequenceDiagram
actor User
participant PdfReaderUI
participant eventBus
participant PdfViewer
participant DOM_thumbnails as DOM_thumbnailsContainer
participant CSS
User->>PdfReaderUI: Click rotate button
PdfReaderUI->>PdfViewer: Change page rotation
PdfViewer->>eventBus: Emit rotationchanging(evt)
eventBus->>PdfReaderUI: rotationchanging listener invoked
PdfReaderUI->>DOM_thumbnails: Query .bb-view-thumbnails
alt thumbnailsContainer exists
PdfReaderUI->>DOM_thumbnails: Set style --thumb-rotate = pagesRotation + deg
DOM_thumbnails->>CSS: Updated custom property --thumb-rotate
CSS->>DOM_thumbnails: Apply transform rotate(var(--thumb-rotate)) to .bb-view-thumbnail-item
else thumbnailsContainer missing
PdfReaderUI-->>PdfReaderUI: No thumbnail rotation applied
end
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
resetThumbnailsView,pdfViewer.getPagesOverview().map(...)is used purely for side effects; consider switching toforEachto better reflect intent and avoid implying a returned array is used. resetThumbnailsViewassumes.bb-view-thumbnailsexists; it would be safer to guard against a nullthumbnailsContainerbefore appending children or attaching event handlers to avoid runtime errors in edge cases.- If
resetThumbnailsViewis intended to be reused, you may want to clear existing thumbnail elements and/or previous click handlers before repopulating to prevent duplicate items and stacked event listeners.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resetThumbnailsView`, `pdfViewer.getPagesOverview().map(...)` is used purely for side effects; consider switching to `forEach` to better reflect intent and avoid implying a returned array is used.
- `resetThumbnailsView` assumes `.bb-view-thumbnails` exists; it would be safer to guard against a null `thumbnailsContainer` before appending children or attaching event handlers to avoid runtime errors in edge cases.
- If `resetThumbnailsView` is intended to be reused, you may want to clear existing thumbnail elements and/or previous click handlers before repopulating to prevent duplicate items and stacked event listeners.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 thumbnail rotation functionality to the PdfReader component. When the main PDF view is rotated, the thumbnail images now rotate accordingly using CSS transforms and a custom CSS variable.
Key Changes:
- Refactored thumbnail initialization logic into a reusable
resetThumbnailsViewfunction - Added
rotationchangingevent handler to dynamically update thumbnail rotation via CSS variables - Introduced CSS custom property
--thumb-rotateto control thumbnail image rotation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Extracted thumbnail initialization into resetThumbnailsView function and added rotation event handler to update CSS variable when pages rotate |
| PdfReader.razor.css | Added --thumb-rotate CSS variable and applied transform: rotate() to thumbnail images |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| const resetThumbnailsView = (el, pdfViewer) => { | ||
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); |
There was a problem hiding this comment.
The resetThumbnailsView function doesn't clear existing thumbnails before recreating them. If this function is called multiple times (e.g., when rotation changes or PDF is reloaded), it will keep appending new thumbnails to the container, leading to duplicates.
Add thumbnailsContainer.innerHTML = ''; at the beginning of the function to clear existing content before regenerating thumbnails.
Suggested change
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); | |
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); | |
| thumbnailsContainer.innerHTML = ''; |
| } | ||
|
|
||
| .bb-view-thumbnails { | ||
| --thumb-rotate: 0; |
There was a problem hiding this comment.
The default value for --thumb-rotate is set to 0 (unitless), but it's used as rotate(var(--thumb-rotate)) which expects a value with units. While CSS is lenient and treats rotate(0) the same as rotate(0deg), for consistency and clarity, consider setting the default to 0deg instead of 0.
Suggested change
| --thumb-rotate: 0; | |
| --thumb-rotate: 0deg; |
Comment on lines
+209
to
+211
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); | ||
| if (thumbnailsContainer) { | ||
| thumbnailsContainer.style.setProperty('--thumb-rotate', `${evt.pagesRotation}deg`); |
There was a problem hiding this comment.
The rotation event handler only updates the CSS variable when thumbnails exist, but it should also check if options.enableThumbnails is true, similar to how other thumbnail-related code does in lines 129-131 and 145-155. Without this check, the handler will attempt to query for thumbnails even when the thumbnails feature is disabled, which is unnecessary overhead.
Consider adding: if (options.enableThumbnails) { ... } around the entire handler body.
Suggested change
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); | |
| if (thumbnailsContainer) { | |
| thumbnailsContainer.style.setProperty('--thumb-rotate', `${evt.pagesRotation}deg`); | |
| if (options.enableThumbnails) { | |
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); | |
| if (thumbnailsContainer) { | |
| thumbnailsContainer.style.setProperty('--thumb-rotate', `${evt.pagesRotation}deg`); | |
| } |
Comment on lines
+234
to
+245
| EventHandler.on(thumbnailsContainer, "click", ".bb-view-thumbnail-item", e => { | ||
| const active = thumbnailsContainer.querySelector('.active'); | ||
| if (active) { | ||
| active.classList.remove('active'); | ||
| } | ||
|
|
||
| const item = e.delegateTarget; | ||
| item.classList.add("active"); | ||
|
|
||
| const index = parseInt(item.getAttribute("data-bb-page")) || 1; | ||
| pdfViewer.currentPageNumber = index; | ||
| }); |
There was a problem hiding this comment.
The event handler is registered each time resetThumbnailsView is called, but it's never removed before re-registration. This will create duplicate event handlers if resetThumbnailsView is called multiple times (e.g., if the PDF is reloaded or thumbnails are regenerated).
Consider either:
- Removing the existing handler before registering a new one:
EventHandler.off(thumbnailsContainer, "click")beforeEventHandler.on(...) - Moving this event handler registration outside of
resetThumbnailsViewto ensure it's only registered once
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 #724
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add support for synchronized rotation of PDF thumbnails in the PdfReader component and refactor thumbnail initialization into a reusable helper.
New Features:
Enhancements: