Skip to content

Commit

Permalink
Simplify the sidebar app side of sidebar <-> host frame messaging
Browse files Browse the repository at this point in the history
Previously the communication between the sidebar and host frame was
implemented by a shared AnnotationSync class, with a 'crossframe'
service which abstracted the event bus on each side.

This design however assumed that both sides wanted to listen to
the same messages and react to them in similar ways. This is not
the case, especially given the change to use Redux for state
management in the sidebar app.

This commit replaces the crossframe service and AnnotationSync class
with a new 'frameSync' service which implements only the event
listeners and sidebar -> page RPC calls that are actually needed.

As a result, local annotation tags for annotations loaded via the API
can now be assigned by the reducer in the Redux store, making this
easier to test and getting us another step closer to making Annotation
objects immutable in the sidebar app.
  • Loading branch information
robertknight committed Sep 21, 2016
1 parent 42a61c6 commit 55093eb
Show file tree
Hide file tree
Showing 15 changed files with 525 additions and 238 deletions.
5 changes: 4 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
{
"extends": "hypothesis"
"extends": "hypothesis",
"globals": {
"Set": false
}
}
12 changes: 7 additions & 5 deletions h/static/scripts/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,11 @@ function configureRoutes($routeProvider) {
}

// @ngInject
function setupCrossFrame(crossframe) {
return crossframe.connect();
function setupFrameSync(frameSync) {
// Setup the connection to the frame hosting the sidebar app.
// This should only be done if this is the sidebar app, not the stream or
// standalone annotation pages.
return frameSync.connect();
}

// @ngInject
Expand Down Expand Up @@ -173,11 +176,11 @@ module.exports = angular.module('h', [
.service('annotationUI', require('./annotation-ui'))
.service('auth', require('./auth').service)
.service('bridge', require('./bridge'))
.service('crossframe', require('./cross-frame'))
.service('drafts', require('./drafts'))
.service('features', require('./features'))
.service('flash', require('./flash'))
.service('formRespond', require('./form-respond'))
.service('frameSync', require('./frame-sync').default)
.service('groups', require('./groups'))
.service('host', require('./host'))
.service('localStorage', require('./local-storage'))
Expand All @@ -195,7 +198,6 @@ module.exports = angular.module('h', [

.factory('store', require('./store'))

.value('AnnotationSync', require('./annotation-sync'))
.value('AnnotationUISync', require('./annotation-ui-sync'))
.value('Discovery', require('./discovery'))
.value('ExcerptOverflowMonitor', require('./directive/excerpt-overflow-monitor'))
Expand All @@ -209,7 +211,7 @@ module.exports = angular.module('h', [
.config(configureLocation)
.config(configureRoutes)

.run(setupCrossFrame)
.run(setupFrameSync)
.run(setupHttp);

processAppOpts();
Expand Down
80 changes: 0 additions & 80 deletions h/static/scripts/cross-frame.coffee

This file was deleted.

4 changes: 2 additions & 2 deletions h/static/scripts/directive/help-panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ module.exports = function () {
bindToController: true,
controllerAs: 'vm',
// @ngInject
controller: function ($scope, $window, crossframe, serviceUrl) {
controller: function ($scope, $window, frameSync, serviceUrl) {
this.userAgent = $window.navigator.userAgent;
this.version = '__VERSION__'; // replaced by versionify
this.dateTime = new Date();
this.serviceUrl = serviceUrl;

$scope.$watchCollection(
function () {
return crossframe.frames;
return frameSync.frames;
},
function (frames) {
if (frames.length === 0) {
Expand Down
4 changes: 2 additions & 2 deletions h/static/scripts/directive/share-dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
var VIA_PREFIX = 'https://via.hypothes.is/';

// @ngInject
function ShareDialogController($scope, $element, crossframe) {
function ShareDialogController($scope, $element, frameSync) {
var self = this;

function updateViaLink(frames) {
Expand All @@ -24,7 +24,7 @@ function ShareDialogController($scope, $element, crossframe) {
viaInput.focus();
viaInput.select();

$scope.$watchCollection(function () { return crossframe.frames; },
$scope.$watchCollection(function () { return frameSync.frames; },
updateViaLink);
}

Expand Down
10 changes: 5 additions & 5 deletions h/static/scripts/directive/test/share-dialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,28 @@ var angular = require('angular');
var util = require('./util');

describe('shareDialog', function () {
var fakeCrossFrame;
var fakeFrameSync;

beforeEach(function () {
fakeCrossFrame = { frames: [] };
fakeFrameSync = { frames: [] };

angular.module('h', [])
.directive('shareDialog', require('../share-dialog'))
.value('crossframe', fakeCrossFrame)
.value('frameSync', fakeFrameSync)
.value('urlEncodeFilter', function (val) { return val; });
angular.mock.module('h');
});

it('generates new via link', function () {
var element = util.createDirective(document, 'shareDialog', {});
fakeCrossFrame.frames.push({ uri: 'http://example.com' });
fakeFrameSync.frames.push({ uri: 'http://example.com' });
element.scope.$digest();
assert.equal(element.ctrl.viaPageLink, 'https://via.hypothes.is/http://example.com');
});

it('does not generate new via link if already on via', function () {
var element = util.createDirective(document, 'shareDialog', {});
fakeCrossFrame.frames.push({ uri: 'https://via.hypothes.is/http://example.com' });
fakeFrameSync.frames.push({ uri: 'https://via.hypothes.is/http://example.com' });
element.scope.$digest();
assert.equal(element.ctrl.viaPageLink, 'https://via.hypothes.is/http://example.com');
});
Expand Down
Loading

0 comments on commit 55093eb

Please sign in to comment.