913: Document Viewer improvements #916
Conversation
| filetype: fileType === 'csv' ? 'text/csv' : fileType, | ||
| return { | ||
| fileName: file, | ||
| fileType: fileType === 'csv' ? 'text/csv' : fileType, |
There was a problem hiding this comment.
Typo fixes! This was causing redundant HEAD requests in the UI.
| fileURLToPath(new URL('../../resources/documents', importMetaUrl())), | ||
| { | ||
| cacheControl: true, | ||
| maxAge: 604800000, // 7 days as millis |
There was a problem hiding this comment.
Documents rarely change, so we can tell the browser to cache them.
| }: { | ||
| documents?: IDocument[]; | ||
| closeOverlay: () => void; | ||
| }) { | ||
| const [documentIndex, setDocumentIndex] = useState<number>(0); |
There was a problem hiding this comment.
Interesting story: I tried moving this "current document index" into the parent component and passing it in as a prop, so that the viewer would remember where you were when reopening. Didn't work - it actually made things worse as the prev and next buttons no longer changed the current page.
Why? Because even though the JSX declaring our DocumentViewBox belongs to the parent (MainComponent.tsx), we pass that as children to Overlay component, which is rendered by the grandparent (App.tsx). We can store the state in the parent, and when calling setDocumentIndex the parent is indeed updated, and in theory DocumentViewBox will receive the new prop. But there is nothing to trigger App component to update: its state does not change, it has no props, and it is not using context. So Overlay doesn't update either, meaning it continues to render its now outdated content with the old prop values. Only when you close and re-open it do you see the correct page, which shows that the buttons are actually updating the state.
We cannot simply lift the state and the DocumentViewBox JSX into App, because the documents are fetched in the level/start request which is made by MainComponent. This kind of limitation has come up before - see mainBodyKey state in MainComponent - and is another example of how storing level state in MainComponent with a useState hook, and passing it all down via prop drilling, is making life uncomfortable. We might be better off switching to a context-based store instead, for level state.
There was a problem hiding this comment.
Always teaching!
well explained
| body?: T; | ||
| }; | ||
|
|
||
| async function get( |
There was a problem hiding this comment.
This is all starting to look like axios! But hey, our implementation here is really lightweight and focused.
| const level = validateLevel(res, req.query.level); | ||
| if (level === null) return; |
There was a problem hiding this comment.
I like this pattern
| fileName, | ||
| fileType, | ||
| uri: `${baseUrl}${PATH}/${folder}/${fileName}`, | ||
| }) as IDocument |
There was a problem hiding this comment.
nice that we're using the proper types given by the api
Description
Documents metadata are now returned in the (sandbox) level data payload. This results in two improvements:
This might be easiest viewed per commit, instead of all in one go.
resolves #913
Notes
fileTypeto know what renderer to use, and the HEAD request is not neededgetandpostfunctions (copied over from the cloud infra feature branch)Checklist
Have you done the following?