Fix guest embedding #2340

Merged
merged 1 commit into from Jul 2, 2015

Projects

None yet

3 participants

@JakeHartnell
Contributor

At the hacking the codex event, I found that unless I modified main.js, I couldn't get Epub.js to work as I couldn't instantiate the Annotator.Guest class, getting a nasty 'Annotator is undefined' error, and failing to create a Guest.

The docs currently suggest to assign the constructor function to 'window.hypothesisRole'. This first attempt at addressing the problem simply moves the constructor function into the 'window.hypothesisConfig' options. As such you would simply create a new guest annotator like so:

window.hypothesisConfig = function () {
    return {
        Constructor: Annotator.Guest
    };
};

Thoughts? Other suggestions? This fixes the problem, but perhaps there is more to be done around our embed code?

I should note that Epub.js is not the only project that will be wanting to create Guest annotators for annotating iframes, so it would be nice if we could get this working so we can move forward with Readium.js integrations, etc.

@JakeHartnell JakeHartnell added the WIP label Jun 30, 2015
@tilgovi
Contributor
tilgovi commented Jul 1, 2015

Just lowercase constructor and I think we're good. Update the docs. I like it.
Also, as a good practice, let's delete the property from the options before passing it to the constructor. This is done because the option isn't used by the class that gets constructed, which doesn't need a handle to its own constructor. Best always not to pass superfluous things.

@JakeHartnell JakeHartnell removed the WIP label Jul 1, 2015
@landscape-bot

Code Health
Code quality remained the same when pulling fc7ac61 on hypothesis:fix-guest-embedding into 690143c on hypothesis:master.

@tilgovi tilgovi commented on an outdated diff Jul 1, 2015
h/static/scripts/annotator/main.js
@@ -81,5 +73,6 @@ if (window.hasOwnProperty('hypothesisConfig')) {
}
Annotator.noConflict().$.noConflict(true)(function () {
- window.annotator = new Klass(document.body, options);
+ window.annotator = new options.constructor(document.body, options);
+ delete options.constructor;
@tilgovi
tilgovi Jul 1, 2015 Contributor

👻

Use a temporary variable to store it and then delete it before it's passed. That was the whole point of deleting it ;).

@landscape-bot

Code Health
Code quality remained the same when pulling c2bc2e5 on hypothesis:fix-guest-embedding into 690143c on hypothesis:master.

@tilgovi tilgovi and 1 other commented on an outdated diff Jul 2, 2015
h/static/scripts/annotator/main.js
var options = {
app: jQuery('link[type="application/annotator+html"]').attr('href'),
BucketBar: {container: '.annotator-frame'},
- Toolbar: {container: '.annotator-frame'}
+ Toolbar: {container: '.annotator-frame'},
@tilgovi
tilgovi Jul 2, 2015 Contributor

This is a syntax error.

@JakeHartnell
JakeHartnell Jul 2, 2015 Contributor

My bad, fixed.

@JakeHartnell JakeHartnell Fix guest embedding.
To prevent an Annotator is undefined error, the constructor class is set
by the object returned from the hypothesisConfig function. The documentation
has also been updated.
8f6f667
@tilgovi
Contributor
tilgovi commented Jul 2, 2015

Thanks. This is great. Thank you for the docs updates, too.

@tilgovi tilgovi merged commit 1a35615 into master Jul 2, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@tilgovi tilgovi deleted the fix-guest-embedding branch Jul 2, 2015
@JakeHartnell
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment