Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: refactor and add support for new tab previews #2

Merged
merged 9 commits into from Oct 19, 2021

Conversation

hnb-ku
Copy link
Contributor

@hnb-ku hnb-ku commented Oct 19, 2021

This PR does 4 things.

  1. Changes the inline preview iFrame src from blob to base64, hopefully resolving some CSP / Safari bugs that are not very easy to repro.

  2. adds an option to skip inline previews for a specific PDF. This can be done by adding a space to the file name in the composer. So, if you change

    [file.pdf|attachment](upload://2cLML0SIwebGHDjlKRVzZ3VRv0f.pdf) (524.1 KB)
    

    to

    [ file.pdf|attachment](upload://2cLML0SIwebGHDjlKRVzZ3VRv0f.pdf) (524.1 KB)
    

    Note the space before "file.pdf" No inline preview will be generated. This does not affect how the filename is rendered when the post is cooked or when the file is downloaded.

  3. Add support for new tab PDF previews - toggled via a setting. The default is still inline rendering, but this gives admins the option to load the PDF file previews in a new tab.

  4. If new tab previews are selected, it changes the icon next to PDF attachments from "download" to "external-link" to indicate that it will open in a new tab.

@hnb-ku
Copy link
Contributor Author

hnb-ku commented Oct 19, 2021

@pmusaraj Can you please take a look at this when you get a chance?

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks quite nice @hnb-ku, a few minor notes:

I wouldn't add the feature to skip inline previews using the space before the filename. It feels fragile and hacky. What is the use case for that, to be able to selectively exclude some PDFs from having a preview?

And an unrelated question: what does a preview in another tab look like? It's an image of the PDF, loaded in a separate tab?

@hnb-ku
Copy link
Contributor Author

hnb-ku commented Oct 19, 2021

Thanks for your time @pmusaraj

I wouldn't add the feature to skip inline previews using the space before the filename. It feels fragile and hacky. What is the use case for that, to be able to selectively exclude some PDFs from having a preview?

I fully agree with your sentiment about the space before the file name.

It's been requested a few times.

https://meta.discourse.org/t/inline-pdf-previews/157649/15?u=johani
https://meta.discourse.org/t/inline-pdf-previews/157649/33?u=johani

However, Discourse doesn't differentiate between attachments on their own line and inline attachments - as it does for inline oneboxes.

So, I had to resort to this. It's a power-user feature, and it's a lot simpler than messing around with the rest of the input in the composer to check for inline attachments in a theme component. This component ignores the composer altogether.

Also, please note that the space added only affects the "description" of the file that gets rendered in the post stream. The component also removes that space in the decorator. So, while it is indeed hacky, it won't have any impact on the upload process, the downloaded file, or how Discourse stores it.

And an unrelated question: what does a preview in another tab look like? It's an image of the PDF, loaded in a separate tab?

It's actually the full file loaded as a blob URL that shows in a new tab. The PDF is opened via the native browser PDF reader.

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations @hnb-ku, you've got me convinced.

LGTM!

@hnb-ku
Copy link
Contributor Author

hnb-ku commented Oct 19, 2021

Cheers @pmusaraj 🙏

Any chance you can merge this so I can update the topic on Meta?

@pmusaraj pmusaraj merged commit dd96fee into discourse:main Oct 19, 2021
@hnb-ku hnb-ku deleted the dev-refactor branch October 20, 2021 09:19
eviltrout pushed a commit that referenced this pull request Oct 20, 2021
* revert back to using blob urls and clean up

* src is not available yet

Co-authored-by: Joe <johani@discourse.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants