PDF Viewer: embedded viewer options #6800
Conversation
| hoverColor?: string; | ||
| } | ||
|
|
||
| export function Button({ onClick, icon, name, size, color, hoverColor }: ButtonProps) { |
There was a problem hiding this comment.
That should be private, so don't export
| import { useRef, useEffect, MutableRefObject } from 'react'; | ||
| import { ScaledSize } from '../pdfSource'; | ||
| import { useRef, useEffect, MutableRefObject, useState } from 'react'; | ||
| import { ScaledSize, PdfData } from '../pdfSource'; |
There was a problem hiding this comment.
One thing about computers is that pretty much everything is "data" so creating a SomethingData class doesn't mean much. What is PdfData? Isn't it a wrapper around the PDF document? Then call it "PdfDocument". Also please create a separate file for it called PdfDocument.ts. Put ScaledSize in a separate types.ts file.
|
|
||
| export interface ScrollSaver { | ||
| container: MutableRefObject<HTMLElement>; | ||
| scaledSize: ScaledSize; | ||
| pdfId: string; | ||
| rememberScroll: boolean; | ||
| onActivePageChange?: (activePage: number)=> void; | ||
| pdf: PdfData; |
|
|
||
| export interface ScrollSaver { | ||
| container: MutableRefObject<HTMLElement>; | ||
| scaledSize: ScaledSize; | ||
| pdfId: string; | ||
| rememberScroll: boolean; | ||
| onActivePageChange?: (activePage: number)=> void; |
There was a problem hiding this comment.
Make "onActivePageChange" required. As much as possible avoid optional stuff, especially when it's not really optional in your code (since you always set it).
|
|
||
| export interface ScrollSaver { | ||
| container: MutableRefObject<HTMLElement>; | ||
| scaledSize: ScaledSize; | ||
| pdfId: string; | ||
| rememberScroll: boolean; | ||
| onActivePageChange?: (activePage: number)=> void; | ||
| pdf: PdfData; | ||
| pageGap?: number; |
packages/pdf-viewer/main.tsx
Outdated
| return <MiniViewerApp pdfPath={url} isDarkTheme={appearance === 'dark'} anchorPage={anchorPage ? Number(anchorPage) : null} pdfId={pdfId} />; | ||
| return <MiniViewerApp pdfPath={url} | ||
| isDarkTheme={appearance === 'dark'} | ||
| anchorPage={anchorPage ? Number(anchorPage) : null} |
There was a problem hiding this comment.
Convert anchorPage to a number (or null) at the point where you get the attribute (so above). Otherwise you have to cast it to int everywhere.
packages/pdf-viewer/pdfSource.ts
Outdated
| }; | ||
|
|
||
| public printPdf = () => { | ||
| const frame = document.createElement('iframe'); |
There was a problem hiding this comment.
Pass "document" to the PdfData (now PdfDocument) class and make it a private property. In general class code shouldn't access the DOM directly or any global variable, as that makes it more difficult to test the code.
| icon?: IconDefinition; | ||
| onClick?: ()=> void; | ||
| name?: string; | ||
| size?: number; | ||
| color?: string; | ||
| hoverColor?: string; |
There was a problem hiding this comment.
Why are these all optional? You're setting al of them every time so make them required. Required properties are easier to deal with, especially when refactoring code. Optional properties are good mostly for public APIs, but for internal code it's rarely needed and doesn't help much.
|
Ok let's merge |
This is a the newer version of previously opened PR #6750.
I created a new PR since my updates were on a separate branch and the merge conflicts were too must to handle for this.
This should not be an issue since code review has not yet started for this feature.
Features added to the viewer
Screenshot