Issue-836 Document Types#1072
Conversation
Adds a `layout` select field (Download Link / Embed) to the Document block so editors can choose how a file is presented. Replaces the hardcoded MIME type allowlist with a hook that rejects only images and videos, directing those to the Media collection instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The layout column was already added by 20260502_155913, so 20260509_162047 caused a duplicate column error on deploy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
20260502_155913 already adds the layout column, but later migration snapshots didn't carry it forward. 20260509_170253 was auto-generated as a result. Making it a no-op so it doesn't fail on prod while its JSON snapshot advances the tracked schema state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Preview deployment: https://issue-836.preview.avy-fx.org |
busbyk
left a comment
busbyk
left a comment
There was a problem hiding this comment.
Thanks for the contribution @scotthillson!
I've requested a few changes. Let me know if you have any questions.
| required: true, | ||
| }, | ||
| { | ||
| name: 'layout', |
There was a problem hiding this comment.
Can we name this displayAs instead? I think that might be clearer to editors.
| if (resolvedLayout === 'download') { | ||
| return ( | ||
| <div className={cn('my-4', { container: isLayoutBlock })}> | ||
| <a |
There was a problem hiding this comment.
Clicking this triggers the nextjs-toploader loader:
If you add target="_blank" we can avoid that.
There was a problem hiding this comment.
I just added to our docs/coding-guide.md to outline our preferences for migration naming and resolving migration conflicts when merging in main has new migrations. Can you please follow those instructions and update this PR's migrations accordingly? #1077
| { | ||
| pattern: /\bALTER\b/i, | ||
| // ALTER TABLE ... ADD is always safe (additive); only flag destructive variants | ||
| pattern: /\bALTER\b(?!.*\bADD\b)/i, |
There was a problem hiding this comment.
Oo, I think this makes sense. But could you break this out into a separate PR or remove?
If the only reason for this change was to be able to commit your changes, we suggest using --no-verify after reviewing to make sure your migration isn't actually destructive. Not perfect, but our goal was to be overly cautious in flagging potentially destructive migrations and then require a dev to review before committing.
Line 64 in c0b1916
| return ( | ||
| <div className={cn('my-4', { container: isLayoutBlock })}> | ||
| <iframe src={src} width="100%" height="600px" title="Document PDF" /> | ||
| <iframe src={src} width="100%" height="600px" title="Document" /> |
There was a problem hiding this comment.
If there is no browser viewer for a given file type and the layout is set to embed, the file is automatically downloaded when visiting the page that the DocumentBlock is on:
I can't find the behavior documented anywhere but it seems that if we set an iframe's src attribute to a file with a MIME type that it does not have a built-in viewer for, the file is automatically downloaded when the iframe is rendered. We definitely don't want this behavior so we need to account for this.
It seems that PDF, HTML, Plain text, and XML/KML can all be rendered in an iframe. So we need conditional logic in the collection that only allows setting the displayAs field to embed when the file's MIME type is one of ['application/pdf', 'text/html', 'text/plain', 'text/xml', 'application/xml', 'application/vnd.google-earth.kml+xml', '.kml']. If we discover there are other MIME types that browsers can reliably render, we could add to that list.
| const filename = document.filename ?? 'Download' | ||
|
|
||
| // Treat missing layout as 'embed' so existing blocks keep their current behavior | ||
| const resolvedLayout = layout ?? 'embed' |
There was a problem hiding this comment.
This was a good assumption to make but I think we should fallback to download due to the browser behavior that might result in a file being automatically downloaded if the browser can't render it in an iframe. See my other comment.
We only have one page with DocumentBlocks in production, so this would be easy enough to go and update manually after we deploy this.
2 participants
Description
Allow all file types in Documents except for images and videos. The Media collection is for images and videos. See issue for more detail.
Related Issues
#836
Key Changes
Adds a layout field to the Document block so editors can choose between embedding a PDF in an iframe or showing a download link button. Also removes the hardcoded mimeTypes allowlist from the Documents collection and replaces it with a validation hook that blocks only images and videos, allowing any other file type (PDFs, KML, XML, etc.) to be uploaded without needing to enumerate every MIME type.
The iframe title was generalized from "Document PDF" to "Document" since non-PDF files can now be embedded.
How to test
Upload a PDF to the Documents collection — should succeed
Try to upload an image file into documents, you'll see a toast error
Screenshots / Demo video
Migration Explanation
Adds a layout column (nullable select) to the blocks_document table. No data is destroyed; existing rows get null which the component treats as embed.