-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add enable-annotation
attribute to managed iframes to opt-in to annotation tools
#404
Conversation
@danielweck |
js/helpers.js
Outdated
@@ -716,13 +716,18 @@ Helpers.loadTemplate = function (name, params) { | |||
*/ | |||
Helpers.loadTemplate.cache = { | |||
"fixed_book_frame": '<div id="fixed-book-frame" class="clearfix book-frame fixed-book-frame"></div>', | |||
|
|||
"single_page_frame": '<div><div id="scaler"><iframe allowfullscreen="allowfullscreen" scrolling="no" class="iframe-fixed"></iframe></div></div>', | |||
"single_page_frame": '<div><div id="scaler"><iframe enable-annotation allowfullscreen="allowfullscreen" scrolling="no" class="iframe-fixed"></iframe></div></div>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please use the XHTML convention for no-value "boolean" attributes, i.e. enable-annotation="enable-annotation"
(to be consistent with our existing allowfullscreen="allowfullscreen"
attribute)
js/helpers.js
Outdated
//"single_page_frame" : '<div><iframe scrolling="no" class="iframe-fixed" id="scaler"></iframe></div>', | ||
|
||
"scrolled_book_frame": '<div id="reflowable-book-frame" class="clearfix book-frame reflowable-book-frame"><div id="scrolled-content-frame"></div></div>', | ||
"reflowable_book_frame": '<div id="reflowable-book-frame" class="clearfix book-frame reflowable-book-frame"></div>', | ||
"reflowable_book_page_frame": '<div id="reflowable-content-frame" class="reflowable-content-frame"><iframe allowfullscreen="allowfullscreen" scrolling="no" id="epubContentIframe"></iframe></div>' | ||
"reflowable_book_page_frame": '<div id="reflowable-content-frame" class="reflowable-content-frame"><iframe enable-annotation allowfullscreen="allowfullscreen" scrolling="no" id="epubContentIframe"></iframe></div>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Copied here:
Can we please use the XHTML convention for no-value "boolean" attributes, i.e. enable-annotation="enable-annotation"
(to be consistent with our existing allowfullscreen="allowfullscreen"
attribute)
I think that's fine. Please just consider using the appropriate XHTML syntax, or at least use a consistent syntax for all attributes on the iframe element. :) |
…otation tools More info: hypothesis/client#533
f4a9d31
to
c57d8e7
Compare
@danielweck |
More info:
hypothesis/client#533
Due to possible timing issues this attribute must be added as soon as the iframe element hits the DOM. The easiest way to do that was to add the attribute to the template markup.