Conversation
|
Size Change: +776 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
I've been investigating what can be done for browsers that don't support embedded PDFs. There's no good option here. Broadly speaking, browsers running on mobile devices are the ones that have a problem.
Unfortunately, there's no such thing as reliable feature detection for embedded PDF support, which gives us a few options:
1c40e28 implements the third option, but I'm interested in arguments either way. 🙂 |
|
The help text is a little bit wordy, I'd appreciate some thoughts on making it less so.
|
|
Do one of these work for you? Depending on what terms you prefer: Note: Most mobile devices do not display embedded PDFs. or Most phone and tablet browsers won't display embedded PDFs. |
|
Perfect, thanks for your help, @michelleweber! |
|
This tests well for me, especially in Chrome, Firefox, Safari and Edge on MacOS. IE11 on a Windows 10 VM with the Adobe Acrobat Reader and plugin installed would try to embed the PDF but appear to get stuck on the "initializing" display of the embedded object. Testing via Browserstack using Windows 10 and IE11 everything worked as expected. So it might have been something funky with my VM. The various mobile and tablet browsers I could test, both on iOS and Android, worked as described. |
| function render_block_core_file( $attributes, $content ) { | ||
| $script = ''; | ||
| if ( ! empty( $attributes['showInlineEmbed'] ) ) { | ||
| $script = <<<HTML |
There was a problem hiding this comment.
Why is this script not an external script that is set as WP_Block_Type::$script when registering the block type below? Or alternatively, why not make it an external script and call wp_enqueue_script if ! empty( $attributes['showInlineEmbed'] )?
There was a problem hiding this comment.
Also, as an external script it can be de-duped with packages/block-library/src/file/utils.js
There was a problem hiding this comment.
wp_enqueue_script() is probably the best option for de-duplicating the code, but I'm not aware of that method being used for any other blocks. (Or even the script property being used, for that matter.)
It seems like it would need a special case to keep it unbundled, and possibly some additional magic when it's merged to Core?
There was a problem hiding this comment.
2b239f3 is a brief exploration of this: I don't mind it for this PR, I'm not wild about it as a generic solution.
There was a problem hiding this comment.
I see render_callback as a function that the sole purpose is to render (return) the content of the block and not trigger a side effect elsewhere. Using wp_enqueue_script creates an indirection that is very hard to follow.
I think a lot of people ask ways for providing frontend scripts for blocks and it could be a property in register_block_type (the equivalent to editor_script => frontend_script).
For the purpose of this PR specifically, I don't mind the hack, whether it's inline or wp_enqueue_script, both are not ideal solution for me but ok.
Co-Authored-By: Michelle W. <michelleweber@users.noreply.github.com>
Co-authored-by: Pascal Birchler <pascalb@google.com>
|
I'm really digging this embed viewer! 👍 I ran through some quick tests. Embed reacts a little funky with alignments
Viewer menu is lost at minimum height
Embed is lost when grouping the block
|
|
Thanks for the testing, @mapk!
These both had the same root cause, fixed in 5739c88.
This appears to be a bug in Chrome's default PDF viewer. They're working on an upgrade to the PDF viewer (you can see it by installing Chrome Canary and enabling this flag: |
|
Thanks @pento! Just to clarify, we don't have any control over the browser-based embed UI, correct? The colors (dark grey) and icons are all beyond our control. If that's the case, this looks good to me. Adding an embed option to this block is a cool feature that I imagine will be useful for many. :) |
That's correct, the embed UI is determined entirely by the browser (or in the case of Windows/IE, Adobe Acrobat 🙃). |
| "source": "html", | ||
| "selector": "a[download]" | ||
| }, | ||
| "showInlineEmbed": { |
There was a problem hiding this comment.
Should we call it "embed" or "preview"? I mean it's not that important but we could the same for like images, text .... any file that can be previewed. Something like "quick view" in MacOS
There was a problem hiding this comment.
showPreview or displayPreview perhaps?
| "align": true | ||
| } | ||
| }, | ||
| "script": "utils.js" |
There was a problem hiding this comment.
It seems it's a way to tell pack to generate a separate script? Why not just a package because that's our way of doing this in Gutenberg?
Alternatively, if we implement the frontend_script proposal, we could introduce a convention like this (I see it more as a file in the block folder with the property here though). Having the property here will persist in the block registration... which I think is confusing.
|
Hey Gary @pento |
|
Noting that I've seen Safari 14 on Big Sur have issues with displaying a PDF via I don't know if it also applies to this PR, but thought it worth mentioning for testing. |
Description
Fixes #19077.
This PR adds an option to the File block, when a PDF file is inserted, to show an embedded version of the PDF.
PDFs are a bit of a special case amongst files: they're the only document file type with native embed support in most major browsers (mobile devices excepted).
How has this been tested?
Screenshots
Types of changes
New feature.
Checklist: