Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

Use PDF.js locally from this repo to allow embedding locally uploaded PDFs #197

Merged
merged 13 commits into from
Dec 22, 2016

Conversation

davisshaver
Copy link
Member

@davisshaver davisshaver commented Dec 16, 2016

Cleans up tests for #193

@davisshaver
Copy link
Member Author

Have this pretty close but working on an issue with CI tests. Looks like absolute paths are being passed into the plugin_dir_url().

For instance, on Travis:

Failed asserting that 'http://example.org/wp-content/plugins/shortcake-bakery/' contains "http://example.org/wp-content/plugins/home/travis/build/wp-shortcake/shortcake-bakery/".

Locally:

Failed asserting that 'http://example.org/wp-content/plugins/shortcake-bakery/' contains "http://example.org/wp-content/plugins/var/www/wordpress/wp-content/plugins/shortcake-bakery/".

Tracked it down to the WP_PLUGINS_URL constant referencing the WP test folder in plugin_basename().

@rmccue, was wondering if you've run into this before in your work on plugin paths/sym links.

@davisshaver
Copy link
Member Author

@goldenapples Following up #193 (comment),

I like the idea of including the example docs as a submodule from the gh-pages branch of PDF.js, but that would require some svn hackery when we want to check it into the plugin repo.

Since we only need a sub-folder of gh-pages I think that if we wanted to go down this path we'd need a subtree instead, but that does seem like it'll complicate the merge strategy. What do you think?

@goldenapples
Copy link
Contributor

we'd need a subtree instead

Ooh, I think that would be way too complicated for me. Unless I'm missing something, I think that'd also break the link to the upstream repo? If so, it wouldn't actually simplify things here. I was thinking something like this:

  • add the entire repo of https://github.com/mozilla/pdf.js as a submodule in assets/lib.
  • set the HEAD of that repo to the current HEAD of /tree/gh-pages in the branch, or to a point on that branch corresponding to a stable release.
  • before checking into the plugin repository, checkout the submodules and set svn ignore properties on all the directories in that repo other than /web and /build.

@davisshaver
Copy link
Member Author

@goldenapples Cleaned it up to use submodule but still fails CI due to the plugin basename issue 🐛

@@ -19,7 +19,7 @@ The follow shortcodes are now available for your use within the content field:
- Facebook `[facebook url="https://www.facebook.com/willpd/posts/1001217146572688"]`
- iFrames (requires code-level configuration of accepted domains) `[iframe src="http://www.buzzfeed.com"]`
- Infogram `[infogram url="http://infogr.am/washington_marijuana_sales"]`
- PDF's (requires PDF served from domain with `Access-Control-Allow-Origin` header) `[pdf url="https://assets.fusion.net/edit/pdfs/the_interview_budget_excerpts.pdf"]`
- PDF's (requires PDF be served locally or from domain with `Access-Control-Allow-Origin` header) `[pdf url="https://assets.fusion.net/edit/pdfs/the_interview_budget_excerpts.pdf"]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note: This README file is built from the readme.txt file (using the grunt readme task), so this change should be made there as well.

@davisshaver davisshaver changed the base branch from add-embed-filter to master December 19, 2016 19:29
Davis Shaver and others added 2 commits December 19, 2016 14:38
Prevents ugly failures caused by the symlink in the test suite.
@goldenapples
Copy link
Contributor

I like this as is.

I don't think we need to really test that the plugin url path matches what we'd expect, since that's outside of the scope of this plugin. I think it would be reasonable for the test suite to just test that the pdf markup contains SHORTCAKE_BAKERY_URL_ROOT . 'assets/lib/pdfjs/web/viewer.html'.

That said, doing something like what I did in #202 should make the tests here pass, if you'd like to merge that into this branch...

@davisshaver
Copy link
Member Author

Thanks for the assist on test coverage @goldenapples. Feels good for merge. Hat tip to @rmccue for the initial PR!

Copy link
Contributor

@goldenapples goldenapples left a comment

Choose a reason for hiding this comment

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

I think this looks good. I'd be happy to merge this as an enhancement for the next release, unless anyone has objections.

If we merge this, I think it'd be good to follow to up with some related enhancements - like maybe hook into the attachment selector to pick a local PDF or upload a PDF, or enable the CORS proxying of remote PDFs. But this makes embedding pdfs a lot easier than previously, since they can just be uploaded to the media library and embedded from the URL there.

@davisshaver
Copy link
Member Author

Good idea about the PDF attachment, hopefully we can tap into some of the recent enhancements.

@goldenapples goldenapples added this to the 0.2.0 milestone Dec 22, 2016
@goldenapples goldenapples changed the title PDF Local for CI Use PDF.js locally from this repo to allow embedding locally uploaded PDFs Dec 22, 2016
@goldenapples
Copy link
Contributor

Connected to #192.

@goldenapples goldenapples merged commit 91137ce into master Dec 22, 2016
@goldenapples goldenapples deleted the pdf-local branch December 22, 2016 22:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants