Skip to content
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

Show the shorter version of the empty annotations/notes message when … #611

Merged
merged 1 commit into from
Dec 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/sidebar/components/selection-tabs.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
'use strict';

var sessionUtil = require('../util/session-util');
var uiConstants = require('../ui-constants');

module.exports = {
controllerAs: 'vm',
//@ngInject
controller: function ($element, annotationUI, features, settings) {
controller: function ($element, annotationUI, features, session, settings) {
this.TAB_ANNOTATIONS = uiConstants.TAB_ANNOTATIONS;
this.TAB_NOTES = uiConstants.TAB_NOTES;
this.TAB_ORPHANS = uiConstants.TAB_ORPHANS;
Expand All @@ -29,6 +30,10 @@ module.exports = {
return this.selectedTab === this.TAB_NOTES &&
this.totalNotes === 0;
};

this.showSidebarTutorial = function () {
return sessionUtil.shouldShowSidebarTutorial(session.state);
};
},
bindings: {
isLoading: '<',
Expand Down
9 changes: 3 additions & 6 deletions src/sidebar/components/sidebar-tutorial.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
'use strict';

var sessionUtil = require('../util/session-util');

// @ngInject
function SidebarTutorialController(session, settings) {
this.cleanOnboardingThemeEnabled = settings.enableCleanOnboardingTheme;

this.showSidebarTutorial = function () {
if (session.state.preferences) {
if (session.state.preferences.show_sidebar_tutorial) {
return true;
}
}
return false;
return sessionUtil.shouldShowSidebarTutorial(session.state);
};

this.dismiss = function () {
Expand Down
113 changes: 101 additions & 12 deletions src/sidebar/components/test/selection-tabs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ var angular = require('angular');
var util = require('../../directive/test/util');

describe('selectionTabs', function () {
var fakeSession = {
state: {
preferences: {
show_sidebar_tutorial: false,
},
},
};
var fakeSettings = {
enableExperimentalNewNoteButton: false,
};

before(function () {
angular.module('app', [])
.component('selectionTabs', require('../selection-tabs'));
Expand All @@ -15,13 +26,11 @@ describe('selectionTabs', function () {
var fakeFeatures = {
flagEnabled: sinon.stub().returns(true),
};
var fakeSettings = {
enableExperimentalNewNoteButton: true,
};

angular.mock.module('app', {
annotationUI: fakeAnnotationUI,
features: fakeFeatures,
session: fakeSession,
settings: fakeSettings,
});
});
Expand All @@ -33,7 +42,6 @@ describe('selectionTabs', function () {
totalAnnotations: '123',
totalNotes: '456',
});

var tabs = elem[0].querySelectorAll('a');

assert.include(tabs[0].textContent, 'Annotations');
Expand All @@ -48,8 +56,8 @@ describe('selectionTabs', function () {
totalAnnotations: '123',
totalNotes: '456',
});

var tabs = elem[0].querySelectorAll('a');

assert.isTrue(tabs[0].classList.contains('is-selected'));
});

Expand All @@ -59,8 +67,8 @@ describe('selectionTabs', function () {
totalAnnotations: '123',
totalNotes: '456',
});

var tabs = elem[0].querySelectorAll('a');

assert.isTrue(tabs[1].classList.contains('is-selected'));
});

Expand All @@ -70,6 +78,7 @@ describe('selectionTabs', function () {
totalAnnotations: '123',
totalNotes: '456',
});

assert.isFalse(elem[0].querySelectorAll('.selection-tabs')[0].classList.contains('selection-tabs--theme-clean'));
});

Expand All @@ -87,28 +96,108 @@ describe('selectionTabs', function () {
totalAnnotations: '123',
totalNotes: '456',
});

assert.isTrue(elem[0].querySelectorAll('.selection-tabs')[0].classList.contains('selection-tabs--theme-clean'));
});

it('should display the new note button when the notes tab is active', function () {
it('should not display the new new note button when the annotations tab is active', function () {
var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'note',
selectedTab: 'annotation',
totalAnnotations: '123',
totalNotes: '456',
});
var newNoteElem = elem[0].querySelectorAll('new-note-btn');
assert.equal(newNoteElem.length, 1);

assert.equal(newNoteElem.length, 0);
});

it('should not display the new new note button when the annotations tab is active', function () {
it('should not display the new note button when the notes tab is active and the new note button is disabled', function () {
var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'annotation',
selectedTab: 'note',
totalAnnotations: '123',
totalNotes: '456',
});

var newNoteElem = elem[0].querySelectorAll('new-note-btn');

assert.equal(newNoteElem.length, 0);
});

it('should display the new note button when the notes tab is active and the new note button is enabled', function () {
fakeSettings.enableExperimentalNewNoteButton = true;
var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'note',
totalAnnotations: '123',
totalNotes: '456',
});
var newNoteElem = elem[0].querySelectorAll('new-note-btn');

assert.equal(newNoteElem.length, 1);
});

it('should display the longer version of the no notes message when there are no notes', function () {
fakeSession.state.preferences.show_sidebar_tutorial = false;
fakeSettings.enableExperimentalNewNoteButton = false;

var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'note',
totalAnnotations: '10',
totalNotes: 0,
});
var unavailableMsg = elem[0].querySelector('.annotation-unavailable-message__label');
var unavailableTutorial = elem[0].querySelector('.annotation-unavailable-message__tutorial');
var noteIcon = unavailableTutorial.querySelector('i');

assert.include(unavailableMsg.textContent, 'There are no page notes in this group.');
assert.include(unavailableTutorial.textContent, 'Create one by clicking the');
assert.isTrue(noteIcon.classList.contains('h-icon-note'));
});

it('should display the longer version of the no annotations message when there are no annotations', function () {
fakeSession.state.preferences.show_sidebar_tutorial = false;
fakeSettings.enableExperimentalNewNoteButton = false;

var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'annotation',
totalAnnotations: 0,
totalNotes: '10',
});
var unavailableMsg = elem[0].querySelector('.annotation-unavailable-message__label');
var unavailableTutorial = elem[0].querySelector('.annotation-unavailable-message__tutorial');
var noteIcon = unavailableTutorial.querySelector('i');

assert.include(unavailableMsg.textContent, 'There are no annotations in this group.');
assert.include(unavailableTutorial.textContent, 'Create one by selecting some text and clicking the');
assert.isTrue(noteIcon.classList.contains('h-icon-annotate'));
});

context('when the sidebar tutorial is displayed', function () {
fakeSession.state.preferences.show_sidebar_tutorial = true;

it('should display the shorter version of the no notes message when there are no notes', function () {
var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'note',
totalAnnotations: '10',
totalNotes: 0,
});
var msg = elem[0].querySelector('.annotation-unavailable-message__label');

assert.include(msg.textContent, 'There are no page notes in this group.');
assert.notInclude(msg.textContent, 'Create one by clicking the');
assert.notInclude(msg.textContent, 'Create one by selecting some text and clicking the');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line was included by mistake? You never need to select text to create a note.

});

it('should display the shorter version of the no annotations message when there are no annotations', function () {
var elem = util.createDirective(document, 'selectionTabs', {
selectedTab: 'annotation',
totalAnnotations: 0,
totalNotes: '10',
});
var msg = elem[0].querySelector('.annotation-unavailable-message__label');

assert.include(msg.textContent, 'There are no annotations in this group.');
assert.notInclude(msg.textContent, 'Create one by clicking the');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this line is a mistake as well, since "Create one by clicking the" is only shown in the notes tab.

Just as a suggestion, it might save a few lines and make the tests more readable to extract out the logic for querying the DOM into a helper function which returns some value to indicate the displayed message that you can then test for. Alternatively if the template "logic" is very simple (eg. hiding/showing different elements depending on some controller methods which return a boolean) you can might be able to get away with just testing the controller method's output in different cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this on Slack. This is a check to make sure that the annotations-related message is not showing.

assert.notInclude(msg.textContent, 'Create one by selecting some text and clicking the');
});
});
});
});
9 changes: 0 additions & 9 deletions src/sidebar/components/test/sidebar-tutorial-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,5 @@ describe('SidebarTutorialController', function () {

assert.equal(result, false);
});

it('returns false if session.state is {}', function () {
var session = {state: {}};
var controller = new Controller(session, settings);

var result = controller.showSidebarTutorial();

assert.equal(result, false);
});
});
});
14 changes: 9 additions & 5 deletions src/sidebar/templates/selection-tabs.html
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,21 @@
<p class="annotation-unavailable-message__label">
There are no page notes in this group.
<br />
Create one by clicking the
<i class="help-icon h-icon-note"></i>
button.
<div ng-if="!vm.enableExperimentalNewNoteButton && !vm.showSidebarTutorial()" class="annotation-unavailable-message__tutorial">
Create one by clicking the
<i class="help-icon h-icon-note"></i>
button.
</div>
</p>
</div>
<div ng-if="vm.showAnnotationsUnavailableMessage()" class="annotation-unavailable-message">
<p class="annotation-unavailable-message__label">
There are no annotations in this group.
<br />
Create one by selecting some text and clicking the
<i class="help-icon h-icon-annotate"></i> button.
<div ng-if="!vm.showSidebarTutorial()" class="annotation-unavailable-message__tutorial">
Create one by selecting some text and clicking the
<i class="help-icon h-icon-annotate"></i> button.
</div>
</p>
</div>
</div>
15 changes: 15 additions & 0 deletions src/sidebar/util/session-util.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';

/**
* Returns true if the sidebar tutorial has to be shown to a user for a given session.
*/
function shouldShowSidebarTutorial(sessionState) {
if (sessionState.preferences.show_sidebar_tutorial) {
return true;
}
return false;
}

module.exports = {
shouldShowSidebarTutorial: shouldShowSidebarTutorial,
};
25 changes: 25 additions & 0 deletions src/sidebar/util/test/session-util-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';

var sessionUtil = require('../session-util');

describe('sessionUtil.shouldShowSidebarTutorial', function () {
it('shows sidebar tutorial if the settings object has the show_sidebar_tutorial key set', function () {
var sessionState = {
preferences: {
show_sidebar_tutorial: true,
},
};

assert.isTrue(sessionUtil.shouldShowSidebarTutorial(sessionState));
});

it('hides sidebar tutorial if the settings object does not have the show_sidebar_tutorial key set', function () {
var sessionState = {
preferences: {
show_sidebar_tutorial: false,
},
};

assert.isFalse(sessionUtil.shouldShowSidebarTutorial(sessionState));
});
});