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

wp-shortcake / shortcake-bakery

A fine selection of shortcodes for WordPress

42 stars 16 forks source link

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

Closed davisshaver closed 7 years ago

davisshaver commented 7 years ago

Cleans up tests for https://github.com/wp-shortcake/shortcake-bakery/pull/193

davisshaver commented 7 years ago

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 commented 7 years ago

@goldenapples Following up https://github.com/wp-shortcake/shortcake-bakery/pull/193#issuecomment-268020450,

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 commented 7 years ago

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 commented 7 years ago

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

goldenapples commented 7 years ago

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 commented 7 years ago

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

davisshaver commented 7 years ago

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

goldenapples commented 7 years ago

Connected to #192.