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

add b flag to pax #36

Merged
merged 4 commits into from
Aug 28, 2024
Merged

add b flag to pax #36

merged 4 commits into from
Aug 28, 2024

Conversation

willpuckett
Copy link
Contributor

This PR adds a b flag to pax to set content type on pdf requests so they display in browser instead of downloading.
I also updated to Deno.serve, and changed the markdown parser since the original one wouldn't load when in testing.

@kawarimidoll
Copy link
Owner

Thank you for your PR!

  • I have never used GitHub to store PDF files, so I don't understand why this feature is needed. Can you give me some examples? Once PDF is supported, many other file types might need to be supported as well, but I don’t want to add a lot of filetype-specific code.
  • Please remove .vscode/ directory.
  • Please apply deno fmt.

Copy link
Contributor Author

@willpuckett willpuckett 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 taking the time to review it! I have found pax to be incredibly useful through the years. I usually use it with Deno, but last week I was consolidating some documentation for my printers. I needed to reference other repos, but I didn't want to use submodules because it was too much information and I just wanted to be able to quickly get to a few files. I made markdown documents with the relevant information. Many links were schematics, which tend to be pdfs. They don't display great in the github pdf viewer—it looks pretty blurry when you zoom in, and it only loads the first few pages. Using the github raw link, github has the headers set for it to download, not open in browser, which then involves navigating and opening the file which is far less streamlined.
This behavior is largely confined to pdfs as far as I can tell. Most other raw files will open in browser, so I don't think it's going to result in substantial bloat. I'm imagining that the behaviour of downloading the file is still preferrable by default, so I added it as a flag.

Copy link
Contributor Author

@willpuckett willpuckett left a comment

Choose a reason for hiding this comment

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

(the markdown link in the above response uses a staging version of pax I put at pux.deno.dev)

@kawarimidoll
Copy link
Owner

Thank you for responding and fixing codes.
The test failed due to a change in the type of the result of the handleURL, so I'm checking the test implementation.
If there is no problem, I will marge this PR.

Copy link
Owner

@kawarimidoll kawarimidoll left a comment

Choose a reason for hiding this comment

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

Thank you so much! Nice work!

@kawarimidoll kawarimidoll merged commit 076194c into kawarimidoll:main Aug 28, 2024
5 of 6 checks passed
@willpuckett
Copy link
Contributor Author

Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants