From bd42a394684f0572f02d50b22809feb615252b86 Mon Sep 17 00:00:00 2001 From: polesye Date: Tue, 20 Jan 2015 12:44:51 +0200 Subject: [PATCH] TNL-1078: Lines don't break correctly in Student Notes. --- lms/static/js/edxnotes/models/note.js | 8 +- lms/static/js/edxnotes/utils/utils.js | 24 ++++++ lms/static/js/edxnotes/views/note_item.js | 7 +- lms/static/js/edxnotes/views/shim.js | 45 +++++++--- .../js/spec/edxnotes/models/note_spec.js | 17 ++-- .../js/spec/edxnotes/views/shim_spec.js | 83 +++++++++++++++++++ 6 files changed, 161 insertions(+), 23 deletions(-) create mode 100644 lms/static/js/edxnotes/utils/utils.js diff --git a/lms/static/js/edxnotes/models/note.js b/lms/static/js/edxnotes/models/note.js index 001f20c43d..e9748dafec 100644 --- a/lms/static/js/edxnotes/models/note.js +++ b/lms/static/js/edxnotes/models/note.js @@ -1,6 +1,6 @@ ;(function (define) { 'use strict'; -define(['backbone', 'underscore.string'], function (Backbone) { +define(['backbone', 'js/edxnotes/utils/utils', 'underscore.string'], function (Backbone, Utils) { var NoteModel = Backbone.Model.extend({ defaults: { 'id': null, @@ -42,7 +42,7 @@ define(['backbone', 'underscore.string'], function (Backbone) { } }, - getNoteText: function () { + getQuote: function () { var message = this.get('quote'); if (!this.get('is_expanded') && this.get('show_link')) { @@ -50,6 +50,10 @@ define(['backbone', 'underscore.string'], function (Backbone) { } return message; + }, + + getText: function () { + return Utils.nl2br(this.get('text')); } }); diff --git a/lms/static/js/edxnotes/utils/utils.js b/lms/static/js/edxnotes/utils/utils.js new file mode 100644 index 0000000000..a134f4a9c8 --- /dev/null +++ b/lms/static/js/edxnotes/utils/utils.js @@ -0,0 +1,24 @@ +;(function (define, undefined) { +'use strict'; +define([], function($, _) { + /** + * Replaces all newlines in a string by HTML line breaks. + * @param {String} str The input string. + * @return `String` with '
' instead all newlines (\r\n, \n\r, \n and \r). + * @example + * nl2br("This\r\nis\n\ra\nstring\r") + * Output: + * This
+ * is
+ * a
+ * string
+ */ + var nl2br = function (str) { + return (str + '').replace(/(\r\n|\n\r|\r|\n)/g, '
'); + } + + return { + nl2br: nl2br + }; +}); +}).call(this, define || RequireJS.define); diff --git a/lms/static/js/edxnotes/views/note_item.js b/lms/static/js/edxnotes/views/note_item.js index 9488496c63..9f83a615c1 100644 --- a/lms/static/js/edxnotes/views/note_item.js +++ b/lms/static/js/edxnotes/views/note_item.js @@ -29,9 +29,10 @@ define([ }, getContext: function () { - return $.extend({ - message: this.model.getNoteText() - }, this.model.toJSON()); + return $.extend({}, this.model.toJSON(), { + message: this.model.getQuote(), + text: this.model.getText() + }); }, toggleNote: function () { diff --git a/lms/static/js/edxnotes/views/shim.js b/lms/static/js/edxnotes/views/shim.js index a17fe606ac..b86e5cdd9e 100644 --- a/lms/static/js/edxnotes/views/shim.js +++ b/lms/static/js/edxnotes/views/shim.js @@ -1,6 +1,8 @@ ;(function (define, undefined) { 'use strict'; -define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) { +define([ + 'jquery', 'underscore', 'annotator', 'js/edxnotes/utils/utils' +], function ($, _, Annotator, Utils) { var _t = Annotator._t; /** @@ -115,6 +117,36 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) { '' ].join(''); + /** + * Overrides Annotator._setupViewer to add a "click" event on viewer and to + * improve line breaks. + **/ + Annotator.prototype._setupViewer = function () { + var self = this; + this.viewer = new Annotator.Viewer({readOnly: this.options.readOnly}); + this.viewer.element.on('click', _.bind(this.onNoteClick, this)); + this.viewer.hide() + .on("edit", this.onEditAnnotation) + .on("delete", this.onDeleteAnnotation) + .addField({ + load: function (field, annotation) { + if (annotation.text) { + $(field).html(Utils.nl2br(Annotator.Util.escape(annotation.text))); + } else { + $(field).html('' + _t('No Comment') + ''); + self.publish('annotationViewerTextField', [field, annotation]); + } + } + }) + .element.appendTo(this.wrapper).bind({ + "mouseover": this.clearViewerHideTimer, + "mouseout": this.startViewerHideTimer + }); + return this; + }; + + Annotator.Editor.prototype.isShown = Annotator.Viewer.prototype.isShown; + /** * Modifies Annotator.onHighlightMouseover to avoid showing the viewer if the * editor is opened. @@ -131,17 +163,6 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) { Annotator.prototype._setupViewer ); - /** - * Modifies Annotator._setupViewer to add a "click" event on viewer. - **/ - Annotator.prototype._setupViewer = _.compose( - function () { - this.viewer.element.on('click', _.bind(this.onNoteClick, this)); - return this; - }, - Annotator.prototype._setupViewer - ); - /** * Modifies Annotator._setupWrapper to add a "click" event on '.annotator-hl'. **/ diff --git a/lms/static/js/spec/edxnotes/models/note_spec.js b/lms/static/js/spec/edxnotes/models/note_spec.js index 285bb090d9..4a491c43e5 100644 --- a/lms/static/js/spec/edxnotes/models/note_spec.js +++ b/lms/static/js/spec/edxnotes/models/note_spec.js @@ -5,8 +5,8 @@ define([ describe('EdxNotes NoteModel', function() { beforeEach(function () { this.collection = new NotesCollection([ - {quote: Helpers.LONG_TEXT}, - {quote: Helpers.SHORT_TEXT} + {quote: Helpers.LONG_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'}, + {quote: Helpers.SHORT_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'} ]); }); @@ -17,18 +17,23 @@ define([ expect(this.collection.at(1).get('show_link')).toBeFalsy(); }); - it('can return appropriate note text', function () { + it('can return appropriate `quote`', function () { var model = this.collection.at(0); // is_expanded = false, show_link = true - expect(model.getNoteText()).toBe(Helpers.PRUNED_TEXT); + expect(model.getQuote()).toBe(Helpers.PRUNED_TEXT); model.set('is_expanded', true); // is_expanded = true, show_link = true - expect(model.getNoteText()).toBe(Helpers.LONG_TEXT); + expect(model.getQuote()).toBe(Helpers.LONG_TEXT); model.set('show_link', false); model.set('is_expanded', false); // is_expanded = false, show_link = false - expect(model.getNoteText()).toBe(Helpers.LONG_TEXT); + expect(model.getQuote()).toBe(Helpers.LONG_TEXT); + }); + + it('can return appropriate `text`', function () { + var model = this.collection.at(0); + expect(model.getText()).toBe('text
with
line
breaks
'); }); }); }); diff --git a/lms/static/js/spec/edxnotes/views/shim_spec.js b/lms/static/js/spec/edxnotes/views/shim_spec.js index f1abd7aab9..6d4335391a 100644 --- a/lms/static/js/spec/edxnotes/views/shim_spec.js +++ b/lms/static/js/spec/edxnotes/views/shim_spec.js @@ -135,5 +135,88 @@ define([ 'click', '.annotator-hl' ); }); + + describe('_setupViewer', function () { + var mockViewer = null; + + beforeEach(function () { + var element = $('
'); + mockViewer = { + fields: [], + element: element + }; + + mockViewer.on = jasmine.createSpy().andReturn(mockViewer); + mockViewer.hide = jasmine.createSpy().andReturn(mockViewer); + mockViewer.destroy = jasmine.createSpy().andReturn(mockViewer); + mockViewer.addField = jasmine.createSpy().andCallFake(function (options) { + mockViewer.fields.push(options); + return mockViewer; + }); + + spyOn(element, 'bind').andReturn(element); + spyOn(element, 'appendTo').andReturn(element); + spyOn(Annotator, 'Viewer').andReturn(mockViewer); + + annotators[0]._setupViewer(); + }); + + it('should create a new instance of Annotator.Viewer and set Annotator#viewer', function () { + expect(annotators[0].viewer).toEqual(mockViewer); + }); + + it('should hide the annotator on creation', function () { + expect(mockViewer.hide.callCount).toBe(1); + }); + + it('should setup the default text field', function () { + var args = mockViewer.addField.mostRecentCall.args[0]; + + expect(mockViewer.addField.callCount).toBe(1); + expect(_.isFunction(args.load)).toBeTruthy(); + }); + + it('should set the contents of the field on load', function () { + var field = document.createElement('div'), + annotation = {text: 'text \nwith\r\nline\n\rbreaks \r'}; + + annotators[0].viewer.fields[0].load(field, annotation); + expect($(field).html()).toBe('text
with
line
breaks
'); + }); + + it('should set the contents of the field to placeholder text when empty', function () { + var field = document.createElement('div'), + annotation = {text: ''}; + + annotators[0].viewer.fields[0].load(field, annotation); + expect($(field).html()).toBe('No Comment'); + }); + + it('should setup the default text field to publish an event on load', function () { + var field = document.createElement('div'), + annotation = {text: ''}, + callback = jasmine.createSpy(); + + annotators[0].on('annotationViewerTextField', callback); + annotators[0].viewer.fields[0].load(field, annotation); + expect(callback).toHaveBeenCalledWith(field, annotation); + }); + + it('should subscribe to custom events', function () { + expect(mockViewer.on).toHaveBeenCalledWith('edit', annotators[0].onEditAnnotation); + expect(mockViewer.on).toHaveBeenCalledWith('delete', annotators[0].onDeleteAnnotation); + }); + + it('should bind to browser mouseover and mouseout events', function () { + expect(mockViewer.element.bind).toHaveBeenCalledWith({ + 'mouseover': annotators[0].clearViewerHideTimer, + 'mouseout': annotators[0].startViewerHideTimer + }); + }); + + it('should append the Viewer#element to the Annotator#wrapper', function () { + expect(mockViewer.element.appendTo).toHaveBeenCalledWith(annotators[0].wrapper); + }); + }); }); });