From 8ea80e114ae508586947db614b37db6b35972a9c Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 19 Feb 2016 12:09:46 +0100 Subject: [PATCH 1/3] Accumulate notifications instead of blinking This makes it possible to display multiple notifications. If the options.type is set to "error", it will also add a close button. --- apps/files/js/file-upload.js | 6 +- core/css/styles.css | 18 +++ core/js/js.js | 168 +++++++++++++++++-------- core/js/tests/specs/coreSpec.js | 211 ++++++++++++++++++++++---------- 4 files changed, 288 insertions(+), 115 deletions(-) diff --git a/apps/files/js/file-upload.js b/apps/files/js/file-upload.js index 8ba294e2a7f..bd80afd072c 100644 --- a/apps/files/js/file-upload.js +++ b/apps/files/js/file-upload.js @@ -472,7 +472,11 @@ OC.Upload = { OC.Upload.showUploadCancelMessage(); } else { // HTTP connection problem - OC.Notification.showTemporary(data.errorThrown, {timeout: 10}); + var message = t('files', 'Error uploading file "{fileName}": {message}', { + fileName: data.files[0].name, + message: data.errorThrown + }); + OC.Notification.show(message, {timeout: 0, type: 'error'}); if (data.result) { var result = JSON.parse(data.result); if (result && result[0] && result[0].data && result[0].data.code === 'targetnotfound') { diff --git a/core/css/styles.css b/core/css/styles.css index e78db6fde01..2fd350f3c4a 100644 --- a/core/css/styles.css +++ b/core/css/styles.css @@ -669,6 +669,24 @@ td.avatar { cursor: pointer; margin-left: 1em; } +#notification { + overflow-x: hidden; + overflow-y: auto; + max-height: 100px; +} +#notification .row { + position: relative; +} +#notification .row .close { + display: inline-block; + vertical-align: middle; + position: absolute; + right: 0; + top: 0; +} +#notification .row.closeable { + padding-right: 20px; +} tr .action:not(.permanent), .selectedActions a { diff --git a/core/js/js.js b/core/js/js.js index fac9c45f668..e90ceaf4e18 100644 --- a/core/js/js.js +++ b/core/js/js.js @@ -988,7 +988,11 @@ OC.msg = { OC.Notification={ queuedNotifications: [], getDefaultNotificationFunction: null, - notificationTimer: 0, + + /** + * @type Array. array of notification timers + */ + notificationTimers: [], /** * @param callback @@ -999,25 +1003,64 @@ OC.Notification={ }, /** - * Hides a notification - * @param callback - * @todo Write documentation + * Hides a notification. + * + * If a row is given, only hide that one. + * If no row is given, hide all notifications. + * + * @param {jQuery} [$row] notification row + * @param {Function} [callback] callback */ - hide: function(callback) { - $('#notification').fadeOut('400', function(){ - if (OC.Notification.isHidden()) { - if (OC.Notification.getDefaultNotificationFunction) { - OC.Notification.getDefaultNotificationFunction.call(); - } - } + hide: function($row, callback) { + var self = this; + var $notification = $('#notification'); + + if (_.isFunction($row)) { + // first arg is the callback + callback = $row; + $row = undefined; + } + + if (!$row) { + console.warn('Missing argument $row in OC.Notification.hide() call, caller needs to be adjusted to only dismiss its own notification'); + // assume that the row to be hidden is the first one + $row = $notification.find('.row:first'); + } + + if ($row && $notification.find('.row').length > 1) { + // remove the row directly + $row.remove(); if (callback) { callback.call(); } - $('#notification').empty(); - if(OC.Notification.queuedNotifications.length > 0){ - OC.Notification.showHtml(OC.Notification.queuedNotifications[0]); - OC.Notification.queuedNotifications.shift(); + return; + } + + _.defer(function() { + // fade out is supposed to only fade when there is a single row + // however, some code might call hide() and show() directly after, + // which results in more than one element + // in this case, simply delete that one element that was supposed to + // fade out + // + // FIXME: remove once all callers are adjusted to only hide their own notifications + if ($notification.find('.row').length > 1) { + $row.remove(); + return; } + + // else, fade out whatever was present + $notification.fadeOut('400', function(){ + if (self.isHidden()) { + if (self.getDefaultNotificationFunction) { + self.getDefaultNotificationFunction.call(); + } + } + if (callback) { + callback.call(); + } + $notification.empty(); + }); }); }, @@ -1025,66 +1068,93 @@ OC.Notification={ * Shows a notification as HTML without being sanitized before. * If you pass unsanitized user input this may lead to a XSS vulnerability. * Consider using show() instead of showHTML() + * * @param {string} html Message to display + * @param {Object} [options] options + * @param {string] [options.type] notification type + * @param {int} [options.timeout=0] timeout value, defaults to 0 (permanent) + * @return {jQuery} jQuery element for notification row */ - showHtml: function(html) { - var notification = $('#notification'); - if((notification.filter('span.undo').length == 1) || OC.Notification.isHidden()){ - notification.html(html); - notification.fadeIn().css('display','inline-block'); - }else{ - OC.Notification.queuedNotifications.push(html); + showHtml: function(html, options) { + options = options || {}; + _.defaults(options, { + timeout: 0 + }); + + var self = this; + var $notification = $('#notification'); + if (this.isHidden()) { + $notification.fadeIn().css('display','inline-block'); } + var $row = $('
'); + if (options.type) { + $row.addClass('type-' + options.type); + } + if (options.type === 'error') { + // add a close button + var $closeButton = $(''); + $closeButton.attr('alt', t('core', 'Dismiss')); + $row.append($closeButton); + $closeButton.one('click', function() { + self.hide($row); + return false; + }); + $row.addClass('closeable'); + } + + $row.prepend(html); + $notification.append($row); + + if(options.timeout > 0) { + // register timeout to vanish notification + this.notificationTimers.push(setTimeout(function() { + self.hide($row); + }, (options.timeout * 1000))); + } + + return $row; }, /** * Shows a sanitized notification + * * @param {string} text Message to display + * @param {Object} [options] options + * @param {string] [options.type] notification type + * @param {int} [options.timeout=0] timeout value, defaults to 0 (permanent) + * @return {jQuery} jQuery element for notification row */ - show: function(text) { - var notification = $('#notification'); - if((notification.filter('span.undo').length == 1) || OC.Notification.isHidden()){ - notification.text(text); - notification.fadeIn().css('display','inline-block'); - }else{ - OC.Notification.queuedNotifications.push($('
').text(text).html()); - } + show: function(text, options) { + return this.showHtml($('
').text(text).html(), options); }, - /** * Shows a notification that disappears after x seconds, default is * 7 seconds + * * @param {string} text Message to show * @param {array} [options] options array * @param {int} [options.timeout=7] timeout in seconds, if this is 0 it will show the message permanently * @param {boolean} [options.isHTML=false] an indicator for HTML notifications (true) or text (false) + * @param {string] [options.type] notification type */ showTemporary: function(text, options) { + var self = this; var defaults = { - isHTML: false, - timeout: 7 - }, - options = options || {}; + isHTML: false, + timeout: 7 + }; + options = options || {}; // merge defaults with passed in options _.defaults(options, defaults); - // clear previous notifications - OC.Notification.hide(); - if(OC.Notification.notificationTimer) { - clearTimeout(OC.Notification.notificationTimer); - } - + var $row; if(options.isHTML) { - OC.Notification.showHtml(text); + $row = this.showHtml(text, options); } else { - OC.Notification.show(text); - } - - if(options.timeout > 0) { - // register timeout to vanish notification - OC.Notification.notificationTimer = setTimeout(OC.Notification.hide, (options.timeout * 1000)); + $row = this.show(text, options); } + return $row; }, /** @@ -1092,7 +1162,7 @@ OC.Notification={ * @return {boolean} */ isHidden: function() { - return ($("#notification").text() === ''); + return !$("#notification").find('.row').length; } }; diff --git a/core/js/tests/specs/coreSpec.js b/core/js/tests/specs/coreSpec.js index 32eb8df32d1..774c2fdc72f 100644 --- a/core/js/tests/specs/coreSpec.js +++ b/core/js/tests/specs/coreSpec.js @@ -747,100 +747,181 @@ describe('Core base tests', function() { }); }); describe('Notifications', function() { - beforeEach(function(){ - notificationMock = sinon.mock(OC.Notification); + var showSpy; + var showHtmlSpy; + var hideSpy; + var clock; + + beforeEach(function() { + clock = sinon.useFakeTimers(); + showSpy = sinon.spy(OC.Notification, 'show'); + showHtmlSpy = sinon.spy(OC.Notification, 'showHtml'); + hideSpy = sinon.spy(OC.Notification, 'hide'); + + $('#testArea').append('
'); }); - afterEach(function(){ - // verify that all expectations are met - notificationMock.verify(); - // restore mocked methods - notificationMock.restore(); - // clean up the global variable - delete notificationMock; + afterEach(function() { + showSpy.restore(); + showHtmlSpy.restore(); + hideSpy.restore(); + // jump past animations + clock.tick(10000); + clock.restore(); }); - it('Should show a plain text notification' , function() { - // one is shown ... - notificationMock.expects('show').once().withExactArgs('My notification test'); - // ... but not the HTML one - notificationMock.expects('showHtml').never(); + describe('showTemporary', function() { + it('shows a plain text notification with default timeout', function() { + var $row = OC.Notification.showTemporary('My notification test'); - OC.Notification.showTemporary('My notification test'); + expect(showSpy.calledOnce).toEqual(true); + expect(showSpy.firstCall.args[0]).toEqual('My notification test'); + expect(showSpy.firstCall.args[1]).toEqual({isHTML: false, timeout: 7}); - // verification is done in afterEach + expect($row).toBeDefined(); + expect($row.text()).toEqual('My notification test'); + }); + it('shows a HTML notification with default timeout', function() { + var $row = OC.Notification.showTemporary('My notification test', { isHTML: true }); + + expect(showSpy.notCalled).toEqual(true); + expect(showHtmlSpy.calledOnce).toEqual(true); + expect(showHtmlSpy.firstCall.args[0]).toEqual('My notification test'); + expect(showHtmlSpy.firstCall.args[1]).toEqual({isHTML: true, timeout: 7}); + + expect($row).toBeDefined(); + expect($row.text()).toEqual('My notification test'); + }); + it('hides itself after 7 seconds', function() { + var $row = OC.Notification.showTemporary(''); + + // travel in time +7000 milliseconds + clock.tick(7000); + + expect(hideSpy.calledOnce).toEqual(true); + expect(hideSpy.firstCall.args[0]).toEqual($row); + }); }); - it('Should show a HTML notification' , function() { - // no plain is shown ... - notificationMock.expects('show').never(); - // ... but one HTML notification - notificationMock.expects('showHtml').once().withExactArgs('My notification test'); + describe('show', function() { + it('hides itself after a given time', function() { + OC.Notification.show('', { timeout: 10 }); - OC.Notification.showTemporary('My notification test', { isHTML: true }); + // travel in time +9 seconds + clock.tick(9000); - // verification is done in afterEach + expect(hideSpy.notCalled).toEqual(true); + + // travel in time +1 seconds + clock.tick(1000); + + expect(hideSpy.calledOnce).toEqual(true); + }); + it('does not hide itself after a given time if a timeout of 0 is defined', function() { + OC.Notification.show('', { timeout: 0 }); + + // travel in time +1000 seconds + clock.tick(1000000); + + expect(hideSpy.notCalled).toEqual(true); + }); + it('does not hide itself if no timeout given to show', function() { + OC.Notification.show(''); + + // travel in time +1000 seconds + clock.tick(1000000); + + expect(hideSpy.notCalled).toEqual(true); + }); }); - it('Should hide previous notification and hide itself after 7 seconds' , function() { - var clock = sinon.useFakeTimers(); + it('cumulates several notifications', function() { + var $row1 = OC.Notification.showTemporary('One'); + var $row2 = OC.Notification.showTemporary('Two', {timeout: 2}); + var $row3 = OC.Notification.showTemporary('Three'); - // previous notifications get hidden - notificationMock.expects('hide').once(); + var $el = $('#notification'); + var $rows = $el.find('.row'); + expect($rows.length).toEqual(3); - OC.Notification.showTemporary(''); + expect($rows.eq(0).is($row1)).toEqual(true); + expect($rows.eq(1).is($row2)).toEqual(true); + expect($rows.eq(2).is($row3)).toEqual(true); - // verify the first call - notificationMock.verify(); + clock.tick(3000); - // expect it a second time - notificationMock.expects('hide').once(); + $rows = $el.find('.row'); + expect($rows.length).toEqual(2); - // travel in time +7000 milliseconds - clock.tick(7000); - - // verification is done in afterEach + expect($rows.eq(0).is($row1)).toEqual(true); + expect($rows.eq(1).is($row3)).toEqual(true); }); - it('Should hide itself after a given time' , function() { - var clock = sinon.useFakeTimers(); + it('shows close button for error types', function() { + var $row = OC.Notification.showTemporary('One'); + var $rowError = OC.Notification.showTemporary('Two', {type: 'error'}); + expect($row.find('.close').length).toEqual(0); + expect($rowError.find('.close').length).toEqual(1); - // previous notifications get hidden - notificationMock.expects('hide').once(); + // after clicking, row is gone + $rowError.find('.close').click(); - OC.Notification.showTemporary('', { timeout: 10 }); + var $rows = $('#notification').find('.row'); + expect($rows.length).toEqual(1); + expect($rows.eq(0).is($row)).toEqual(true); + }); + it('fades out the last notification but not the other ones', function() { + var fadeOutStub = sinon.stub($.fn, 'fadeOut'); + var $row1 = OC.Notification.show('One', {type: 'error'}); + var $row2 = OC.Notification.show('Two', {type: 'error'}); + OC.Notification.showTemporary('Three', {timeout: 2}); - // verify the first call - notificationMock.verify(); + var $el = $('#notification'); + var $rows = $el.find('.row'); + expect($rows.length).toEqual(3); - // expect to not be called after 9 seconds - notificationMock.expects('hide').never(); + clock.tick(3000); - // travel in time +9 seconds - clock.tick(9000); - // verify this - notificationMock.verify(); + $rows = $el.find('.row'); + expect($rows.length).toEqual(2); - // expect the second call one second later - notificationMock.expects('hide').once(); - // travel in time +1 seconds + $row1.find('.close').click(); clock.tick(1000); - // verification is done in afterEach + expect(fadeOutStub.notCalled).toEqual(true); + + $row2.find('.close').click(); + clock.tick(1000); + expect(fadeOutStub.calledOnce).toEqual(true); + + expect($el.is(':empty')).toEqual(false); + fadeOutStub.yield(); + expect($el.is(':empty')).toEqual(true); + + fadeOutStub.restore(); }); - it('Should not hide itself after a given time if a timeout of 0 is defined' , function() { - var clock = sinon.useFakeTimers(); + it('hides the first notification when calling hide without arguments', function() { + var $row1 = OC.Notification.show('One'); + var $row2 = OC.Notification.show('Two'); - // previous notifications get hidden - notificationMock.expects('hide').once(); + var $el = $('#notification'); + var $rows = $el.find('.row'); + expect($rows.length).toEqual(2); - OC.Notification.showTemporary('', { timeout: 0 }); + OC.Notification.hide(); - // verify the first call - notificationMock.verify(); + $rows = $el.find('.row'); + expect($rows.length).toEqual(1); + expect($rows.eq(0).is($row2)).toEqual(true); + }); + it('hides the given notification when calling hide with argument', function() { + var $row1 = OC.Notification.show('One'); + var $row2 = OC.Notification.show('Two'); - // expect to not be called after 1000 seconds - notificationMock.expects('hide').never(); + var $el = $('#notification'); + var $rows = $el.find('.row'); + expect($rows.length).toEqual(2); - // travel in time +1000 seconds - clock.tick(1000000); + OC.Notification.hide($row2); - // verification is done in afterEach + $rows = $el.find('.row'); + expect($rows.length).toEqual(1); + expect($rows.eq(0).is($row1)).toEqual(true); }); }); describe('global ajax errors', function() { From 27544144cec06324a5fe4494034e893b7c5a66ff Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 22 Feb 2016 17:19:08 +0100 Subject: [PATCH 2/3] Fix unit tests affected by side effects The notification tests were not restoring the clock properly, but indirectly helped other tests pass. Since now we're restoring the clock properly, the other tests were fixed to still work. --- apps/files/tests/js/fileactionsSpec.js | 4 +++- apps/systemtags/tests/js/systemtagsinfoviewSpec.js | 3 +++ core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js | 4 +++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/files/tests/js/fileactionsSpec.js b/apps/files/tests/js/fileactionsSpec.js index a905a4d969d..4f4d4d3d197 100644 --- a/apps/files/tests/js/fileactionsSpec.js +++ b/apps/files/tests/js/fileactionsSpec.js @@ -20,9 +20,10 @@ */ describe('OCA.Files.FileActions tests', function() { - var fileList, fileActions; + var fileList, fileActions, clock; beforeEach(function() { + clock = sinon.useFakeTimers(); // init horrible parameters var $body = $('#testArea'); $body.append(''); @@ -63,6 +64,7 @@ describe('OCA.Files.FileActions tests', function() { fileActions = null; fileList.destroy(); fileList = undefined; + clock.restore(); $('#dir, #permissions, #filestable').remove(); }); it('calling clear() clears file actions', function() { diff --git a/apps/systemtags/tests/js/systemtagsinfoviewSpec.js b/apps/systemtags/tests/js/systemtagsinfoviewSpec.js index 0fb4e7b22c2..27724822c2e 100644 --- a/apps/systemtags/tests/js/systemtagsinfoviewSpec.js +++ b/apps/systemtags/tests/js/systemtagsinfoviewSpec.js @@ -22,14 +22,17 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { var isAdminStub; var view; + var clock; beforeEach(function() { + clock = sinon.useFakeTimers(); view = new OCA.SystemTags.SystemTagsInfoView(); $('#testArea').append(view.$el); isAdminStub = sinon.stub(OC, 'isUserAdmin').returns(true); }); afterEach(function() { isAdminStub.restore(); + clock.restore(); view.remove(); view = undefined; }); diff --git a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js b/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js index d62ef672f4d..aadf0de53f2 100644 --- a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js +++ b/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js @@ -20,9 +20,10 @@ */ describe('OC.SystemTags.SystemTagsInputField tests', function() { - var view, select2Stub; + var view, select2Stub, clock; beforeEach(function() { + clock = sinon.useFakeTimers(); var $container = $('
'); select2Stub = sinon.stub($.fn, 'select2'); select2Stub.returnsThis(); @@ -31,6 +32,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { afterEach(function() { select2Stub.restore(); OC.SystemTags.collection.reset(); + clock.restore(); view.remove(); view = undefined; }); From e37372e8832c9d12c0c602356bd7d3a122a2bb61 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 22 Feb 2016 17:25:16 +0100 Subject: [PATCH 3/3] Remove unused update-notification style --- core/css/styles.css | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/css/styles.css b/core/css/styles.css index 2fd350f3c4a..4edb29a8c6e 100644 --- a/core/css/styles.css +++ b/core/css/styles.css @@ -650,7 +650,7 @@ td.avatar { width: 100%; text-align: center; } -#notification, #update-notification { +#notification { margin: 0 auto; max-width: 60%; z-index: 8000; @@ -665,7 +665,7 @@ td.avatar { -ms-filter: "progid:DXImageTransform.Microsoft.Alpha(Opacity=90)"; opacity: .9; } -#notification span, #update-notification span { +#notification span { cursor: pointer; margin-left: 1em; }