Feature/f 35 pdf preview implementation #14
Conversation
| @@ -1,3 +1,8 @@ | |||
| @tailwind base; | |||
| @tailwind components; | |||
| @tailwind utilities; | |||
|
|
|||
| /* PdfViewer.css */ | |||
| canvas.react-pdf__Page__canvas { | |||
There was a problem hiding this comment.
we need to use css here to add padding between pages
|
Yeah I think it's okay, I'm not even sure how you're supposed to respect this rule in a functional component |
There was a problem hiding this comment.
Overall great job! Take a look at my comments, just smal code styling issues I found. Also few things to add here:
- Can you please add the download PDF Functionality?
- Can you think how we can get the documents from this endpoint
https://static.hungermapdata.org/insight-reports/latest/country.jsonand display them in the previewer? - add border to the navbar and the color could be like light gray in light mode and dark grey in dark mode.
But overall quality and functionality is great! Good job!
src/components/Pdf/PdfViewer.tsx
Outdated
| const handleScroll = () => { | ||
| const pages = document.querySelectorAll('.react-pdf__Page'); | ||
| let currentPage = pageNumber; | ||
|
|
||
| pages.forEach((page, index) => { | ||
| const rect = page.getBoundingClientRect(); | ||
| const pageTop = rect.top; | ||
| const pageBottom = rect.bottom; | ||
| const viewportMidpoint = window.innerHeight / 2; | ||
|
|
||
| if (pageTop < viewportMidpoint && pageBottom > viewportMidpoint) { | ||
| currentPage = index + 1; | ||
| } | ||
| }); | ||
|
|
||
| setPageNumber(currentPage); | ||
| }; |
There was a problem hiding this comment.
This function should be exported to an operations file in operations/pdf-previewer/PdfPreviewerOperations.ts
src/components/Pdf/PdfViewer.tsx
Outdated
| const [selectionText, setSelectionText] = useState<string | null>(null); | ||
| const [tooltipPosition, setTooltipPosition] = useState<{ top: number; left: number } | null>(null); | ||
|
|
||
| const handleScroll = () => { |
There was a problem hiding this comment.
precise naming handleDocumentScroll + return type of the function is missing
src/components/Pdf/PdfViewer.tsx
Outdated
| function onDocumentLoadSuccess({ numPages }: { numPages: number }): void { | ||
| setTotalPages(numPages); | ||
| window.addEventListener('scroll', handleScroll); | ||
| } |
There was a problem hiding this comment.
you should not use arrow function declaration and function declaration in the same file. Chose one of both approaches and be consistent
src/components/Pdf/PdfViewer.tsx
Outdated
| function onSelectStart() { | ||
| setSelectionText(null); | ||
| } |
src/components/Pdf/PdfViewer.tsx
Outdated
| window.addEventListener('scroll', handleScroll); | ||
| } | ||
|
|
||
| function onSelectStart() { |
There was a problem hiding this comment.
return type of the function is missing
src/components/Pdf/PdfViewer.tsx
Outdated
| setSelectionText(null); | ||
| } | ||
|
|
||
| function onSelectEnd() { |
| @@ -26,7 +28,7 @@ module.exports = { | |||
| light: { | |||
| colors: { | |||
| primary: { DEFAULT: '#157DBC', foreground: '#F5F5F5' }, | |||
| secondary: '#EEEEEE', | |||
| secondary: '#666666', | |||
There was a problem hiding this comment.
this corresponds to our secondary color used in figma designs
marinovl7
deleted the
feature/f-35-pdf-preview-library-research-and-implementation
branch
@Tschonti please approve if it is ok that I add
"react/jsx-no-bind": "off"to our linter config. I think we need it for defining functions inside components likehandleScroll(seePdfViewer.tsx).