Add two-way iframe communication via postMessage to URL Plugin #70
Conversation
|
In package.json there's a script for "node-qunit-phantomjs", but it's not included as a dep. Can you please add |
rchrd2
left a comment
rchrd2
left a comment
There was a problem hiding this comment.
I think we should put the jquery-1.10.1.js in the same BookReader dir. I think it's maybe the only 3rd party library that's not included, so may as well put with the others.
rchrd2
left a comment
rchrd2
left a comment
There was a problem hiding this comment.
It kind of seems like the iframe communication should be it's own plugin.
I don't see it as being fundamentally part of the URL plugin, but more like an extra feature.
What do you think?
|
My thinking on the iframe communication being its own plugin: I considered it, but it’s kind of tied to the idea of the params path and the URL. I don't see a way that it could be decoupled from that, but maybe I'm missing something. |
|
I should say, it's not IMPOSSIBLE to decouple it, but it would require a bigger rethink of the interface that plugins and other code use to interact with BookReader (i.e. more based on events and less on prototype modification). |
|
But what if it was a plugin that depends on the url plugin? In the comment it could say "depends on plugin.url.js". |
|
I'm not comfortable with having plugin dependencies like that, at least not right now while the plugins are all tightly coupled to the internal workings of BookReader. You'd have one plugin which makes prototype modifications and relies on prototype modifications made by another plugin. Gets pretty gnarly if you ever want to, say, change the URL plugin. |
|
The unfortunately fact is, plugins already have some dependencies. Each is
not 100% isolated.
…On Wed, Apr 25, 2018 at 3:00 PM, Evan Minto ***@***.***> wrote:
I'm not comfortable with having plugin dependencies like that, at least
not right now while the plugins are all tightly coupled to the internal
workings of BookReader. You'd have one plugin which makes prototype
modifications and relies on prototype modifications made by another plugin.
Gets pretty gnarly if you ever want to, say, change the URL plugin.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#70 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALIz4sJDknoKO2VB-fb0YZIRkIbQMzvks5tsPILgaJpZM4Tj_7O>
.
|
WEBDEV-1528
WEBDEV-1528
|
This is ready for re-review, @rchrd2. |
…hen parsing fragment WEBDEV-1528
BookReader/plugins/plugin.url.js
Outdated
| * parent window when pages change, and the parent window can also | ||
| * explicitly request a page change by sending its own message. | ||
| * | ||
| * @param {BookReader} bookReader |
There was a problem hiding this comment.
Suggestion: Throughout the bookreader codebase, an instance of BookReader is usually named br. Rename it to this to be consistent.
BookReader/plugins/plugin.url.js
Outdated
| title += '...'; | ||
| return title; | ||
| }; | ||
| jQuery.extend(true, BookReader.defaultOptions, { |
There was a problem hiding this comment.
Suggestion: In all the other plugins, this function call and the setup method are at the top of the file. This makes it easy to see what the plugin options are.
Can you move this to the top of the file?
BookReader/plugins/plugin.url.js
Outdated
| bookId: '', | ||
| // Defaults can be a urlFragment string | ||
| defaults: null, | ||
| }); |
There was a problem hiding this comment.
The iframe communication should probably also have an option to disable. Like enableIframeCommunication: true
BookReader/plugins/plugin.url.js
Outdated
| */ | ||
| function setUpIframeCommunication(bookReader) { | ||
| // Not embedded, abort | ||
| if (!window.parent) { |
There was a problem hiding this comment.
Note. There's a difference between "embedded" and in an iframe.
There's ui=embed.
You may want to change this comment to be more clear with that. Like Not in iframe, abort
BookReader/plugins/plugin.url.js
Outdated
|
|
||
| /** | ||
| * Set up two-way communication between the BookReader and a parent window | ||
| * (if the reader is embedded via an iframe) |
There was a problem hiding this comment.
See my other note about "embed" vs "iframe" terminology.
2 participants
Also adds some QUnit tests and updates documentation.
Jira: WEBDEV-1528