feat: add vertical scrolling mode for PDF viewer #511
Conversation
- Implemented vertical scrolling mode to display all PDF pages in a continuous scroll view. - Added styles for vertical layout and page number indicators in CSS. - Enhanced JavaScript logic for rendering all pages and tracking current page based on scroll position. - Introduced a toggle button in the toolbar for switching between vertical scroll mode and traditional page mode. - Updated Android integration to support vertical scroll mode and preserve state during mode switching. - Added new vector drawable for the vertical scroll toggle icon.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a vertical scrolling mode for the PDF viewer, expanding the display options and integrating the feature across the JavaScript viewer, CSS styling, and Android integration. Key changes include:
- Implementing vertical scroll mode logic with new rendering functions and scroll handling in index.js.
- Adding corresponding CSS styles and a toggle button in the toolbar.
- Updating Android integration to preserve mode state and respond to vertical scroll events.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| viewer/js/index.js | Added functions to render all pages vertically, including scroll listener setup and toggling logic. |
| viewer/css/pdf_viewer.css | Introduced styles for the vertical scroll container and page indicators. |
| app/src/main/res/values/strings.xml | Added new string resource for vertical scrolling toggle. |
| app/src/main/res/menu/pdf_viewer.xml | Inserted a new menu item for toggling vertical scroll mode. |
| app/src/main/res/drawable/ic_view_agenda_24dp.xml | Added vector drawable for the vertical scroll icon. |
| app/src/main/java/app/grapheneos/pdfviewer/PdfViewer.java | Integrated vertical scroll mode logic into the Android activity, including state preservation and JS interaction. |
| // Calculate appropriate zoom for vertical layout | ||
| const firstPage = await pdfDoc.getPage(1); | ||
| const defaultZoom = getDefaultZoomRatio(firstPage, orientationDegrees); | ||
| const verticalZoom = Math.min(defaultZoom * 0.8, containerWidth / firstPage.getViewport({scale: 1}).width); |
There was a problem hiding this comment.
Consider extracting the magic constant 0.8 into a named constant (e.g., VERTICAL_ZOOM_FACTOR) to improve code clarity and maintainability.
| const verticalZoom = Math.min(defaultZoom * 0.8, containerWidth / firstPage.getViewport({scale: 1}).width); | |
| const verticalZoom = Math.min(defaultZoom * VERTICAL_ZOOM_FACTOR, containerWidth / firstPage.getViewport({scale: 1}).width); |
Copilot uses AI. Check for mistakes.
| padding: 20px; | ||
| width: 100%; | ||
| min-height: 100vh; | ||
| `; |
There was a problem hiding this comment.
[nitpick] The inline CSS styles for the all pages container duplicate styles defined in the CSS file; consider centralizing these styles in the stylesheet to avoid duplication and ease future updates.
Copilot uses AI. Check for mistakes.
feat: add vertical scrolling mode for PDF viewer