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

Feature/static viewer #10

Merged
merged 18 commits into from
Nov 5, 2013
Merged

Feature/static viewer #10

merged 18 commits into from
Nov 5, 2013

Conversation

bormind
Copy link
Contributor

@bormind bormind commented Oct 29, 2013

Hi @aadamowski I would like to merge feature/static_viewer to develop branch

@dmitrym0
Copy link

Just a quick tip, readium-js is a submodule of this project, and readium-shared-js is a submodule of THAT project.

You can invoke this: git submodule update --init --recursive to make sure that all dependencies are fetched correctly.

@aadamowski
Copy link
Contributor

Hello @bormind, thanks for the pull request.

I've tested that the changed code works - however I see significant issues that need to be addressed before merging:

Simplicity of obtaining and putting to use

The idea with attaching readium-js as a Git submodule is good. However, the way it's done here goes against the main aim of the readium-js-viewer project.

The aim of readium-js-viewer is to provide a simplistic, working example of usage of the Readium.js library for any newcoming users.

Therefore, the main requrements for readium-js-viewer are that:

  1. The viewer's scripts reference a single file version of the Readium.js library as simply as possible - they cannot delve into Readium.js internals like this pull request introduces in lib/initialize_rjs_reader.js. I think that for this it's necessary to launch readium-js submodule's Grunt build as a task from readium-js-viewer build (e.g. using grunt.util.spawn) in order to get Readium.js built in readium-js/epub-modules/readium-js/out/, then refer to the built readium-js/epub-modules/readium-js/out/Readium.js file from initialize_rjs_reader.js. One difficulty with this approach may be related to readium-js submodule's NPM dependencies required for building it. A way to launch npm install in readium-js/ from the viewer's Grunt build would be needed, ideally in a cross-platform way (e.g. using a grunt-npm task rather than launching a OS-dependent shell command using Grunt's exec task).
  2. The amount of steps is minimal for getting the readium-js-viewer and getting it to work in one's browser. Ideally, there should be only 3 steps necessary: clone the repo, put the contents on a web server, visit the web server's address in one's browser. This means that build procedure (be it using Grunt or anything) should be completely optional for achieving this minimal task. Until now, it was true - the Grunt build only served the purpose of launching an embedded Node.JS/Express web server and it was just for convenience. There were no additional steps needed related to Git submodules if one only wanted to see how the viewer works. With the proposed changes, one has to at least init/update git submodules (recursively).
  3. The build procedure (whether optional or not) should be fully documented (step by step) in README.md and, before publishing, tested that it works when starting from a freshly cloned repo. All steps, like git submodule update --init --recursive, npm install, installing and launching grunt should be mentioned - there should be no assumption that any one of them is obvious.

The general rule of thumb is that anybody (not only skilled contributors) should be able to clone readium-js-viewer source repo, have a look at the HTML files and scripts that they reference, and see a simplistic usage scenario for the Readium.js library file. Then he/she should be able to look how it works in a browser without having to install Grunt, its NPM depencencies, updating git submodules or any other developer stuff - just copy the files to a web server and point the browser to appropriate URL.

This obviously requires that the readium-js-viewer source repository holds a pre-built version of Readium.js.

Git submodules and Grunt build may serve the purpose of conveniently, automatically updating that pre-built Readium.js library file to the latest (or, for that matter, any other chosen) version, but they should not be a prerequisite to get the viewer running.

Synchronous version of Readium.js library

Apart from demonstrating the AMD/RequireJS version of the Readium.js library, the aim of readium-js-viewer is also to demonstrate the usage of synchronous version (loaded simply through a SCRIPT tag).

The index-sync.html page is intended for that demonstration.

Unfortunately, since the build version of that library file (lib/Readium.syncload.js) has been removed from the source tree, and there's no build procedure to generate it, this example is now left fundamentally broken.

Internal link handling

I didn't yet have the time yet to diagnose this, but internal link handling has stopped working, despite the fact that the employed version of readium-js submodule contains the readium/readium-js@ee36f2f029 commit that implemented support for internal links - this needs investigating.

Symptoms are related to the fact that, without the support from readium/readium-js@ee36f2f029 , internal links are handled by the default browser logic for links and this doesn't work well for EPUBs:

  • exploded EPUBs (e.g. "Linear Algebra") initially exhibit no problems after clicking an internal link, but after using previous/next page navigation buttons a few times, the navigational state gets confused and jumps to incorrect pages
  • zipped EPUBs (e.g. "Zipped EPUB with internal links") don't handle internal links at all (obviously, since they will navigate to locations outside the zipped EPUB file, where nothing useful resides)

@bormind bormind merged commit c4ab301 into develop Nov 5, 2013
@bormind bormind deleted the feature/static_viewer branch November 8, 2013 18:54
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.

4 participants