Skip to content

Commit

Permalink
Replace sidebarWindow property with sidebar.iframe.contentWindow
Browse files Browse the repository at this point in the history
Rather than use a dedicated getter to expose the sidebar frame, it
seemed better on a second look to use the existing `sidebar.iframe`
property that exposes the `<iframe>` element, but document and test that
property.
  • Loading branch information
robertknight committed Jul 25, 2021
1 parent 36fe2c0 commit 4305f25
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 9 deletions.
4 changes: 2 additions & 2 deletions src/annotator/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ function init() {
sidebar = new Sidebar(document.body, eventBus, guest, getConfig('sidebar'));

// Expose sidebar window reference for use by same-origin guest frames.
window_.__hypothesis.sidebarWindow = sidebar.sidebarWindow;
window_.__hypothesis.sidebarWindow = sidebar.iframe.contentWindow;
}

// Clear `annotations` value from the notebook's config to prevent direct-linked
Expand All @@ -77,7 +77,7 @@ function init() {

// Set up communication between this host/guest frame and the sidebar frame.
const sidebarWindow = sidebar
? sidebar.sidebarWindow
? sidebar.iframe.contentWindow
: /** @type {HypothesisWindow} */ (window.parent).__hypothesis
?.sidebarWindow;
if (sidebarWindow) {
Expand Down
12 changes: 5 additions & 7 deletions src/annotator/sidebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,12 @@ export default class Sidebar {
*/
constructor(element, eventBus, guest, config = {}) {
this._emitter = eventBus.createEmitter();

/**
* The `<iframe>` element containing the sidebar application.
*/
this.iframe = createSidebarIframe(config);

this.options = config;

/** @type {BucketBar|null} */
Expand Down Expand Up @@ -192,13 +197,6 @@ export default class Sidebar {
this._setupSidebarEvents();
}

/**
* Return a reference to the `Window` containing the sidebar application.
*/
get sidebarWindow() {
return /** @type {Window} */ (this.iframe.contentWindow);
}

destroy() {
this.bucketBar?.destroy();
this._listeners.removeAll();
Expand Down
10 changes: 10 additions & 0 deletions src/annotator/test/sidebar-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,16 @@ describe('Sidebar', () => {
});
});

describe('#iframe', () => {
it('returns a reference to the `<iframe>` containing the sidebar', () => {
const sidebar = createSidebar();
const iframe = containers[0]
.querySelector('hypothesis-sidebar')
.shadowRoot.querySelector('iframe');
assert.equal(sidebar.iframe, iframe);
});
});

function getConfigString(sidebar) {
return sidebar.iframe.src;
}
Expand Down

0 comments on commit 4305f25

Please sign in to comment.