From e2549fa660630c1345dcdc84be9114ad2e54c0aa Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 14 Aug 2015 15:54:27 +0200 Subject: [PATCH] Improve right sidebar tabs behavior Tab heads are not rendered if only one tab. The tab contents is updated on-demand. This means that if a tab is not visible it is not rendered at first. If the tab was already rendered through switching, its model will not get updated until the next time it becomes visible. This will prevent needless rerendering of invisible tab contents, especially considering that some tabs might need extra ajax requests. --- apps/files/js/detailsview.js | 182 ++++++++++++++++--------- apps/files/js/detailtabview.js | 6 +- apps/files/js/tagsplugin.js | 3 +- apps/files/templates/test.png | Bin 0 -> 2302 bytes apps/files/tests/js/detailsviewSpec.js | 80 +++++++++-- apps/files_sharing/js/share.js | 8 ++ apps/files_sharing/js/sharetabview.js | 9 +- 7 files changed, 202 insertions(+), 86 deletions(-) create mode 100644 apps/files/templates/test.png diff --git a/apps/files/js/detailsview.js b/apps/files/js/detailsview.js index 4df359e4523..a4ebe90cd64 100644 --- a/apps/files/js/detailsview.js +++ b/apps/files/js/detailsview.js @@ -9,23 +9,26 @@ */ (function() { - var TEMPLATE = '
' + - '
' + - '
' + - '
' + - '
    ' + - '
' + - '
' + - '
' + - '
' + - ' ' + + '
' + + '
' + + '
' + + ' {{#if tabHeaders}}' + + '
    ' + + ' {{#each tabHeaders}}' + + '
  • ' + + ' {{label}}' + + '
    • ' + + ' {{/each}}' + + '
' + + ' {{/if}}' + + '
' + + '
' + + '
' + + ' ' + '
'; - var TEMPLATE_TAB_HEADER = - '
  • {{label}}
    • '; - /** * @class OCA.Files.DetailsView * @classdesc @@ -39,7 +42,6 @@ className: 'detailsView', _template: null, - _templateTabHeader: null, /** * List of detail tab views @@ -62,6 +64,11 @@ */ _currentTabId: null, + /** + * Dirty flag, whether the view needs to be rerendered + */ + _dirty: false, + events: { 'click a.close': '_onClose', 'click .tabHeaders .tabHeader': '_onClickTab' @@ -74,8 +81,10 @@ this._tabViews = []; this._detailFileInfoViews = []; + this._dirty = true; + // uncomment to add some dummy tabs for testing - // this._addTestTabs(); + //this._addTestTabs(); }, _onClose: function(event) { @@ -85,28 +94,21 @@ _onClickTab: function(e) { var $target = $(e.target); + e.preventDefault(); if (!$target.hasClass('tabHeader')) { $target = $target.closest('.tabHeader'); } - var tabIndex = $target.attr('data-tabindex'); - var targetTab; - if (_.isUndefined(tabIndex)) { + var tabId = $target.attr('data-tabid'); + if (_.isUndefined(tabId)) { return; } - this.$el.find('.tabsContainer .tab').addClass('hidden'); - targetTab = this._tabViews[tabIndex]; - targetTab.$el.removeClass('hidden'); - - this.$el.find('.tabHeaders li').removeClass('selected'); - $target.addClass('selected'); - - e.preventDefault(); + this.selectTab(tabId); }, _addTestTabs: function() { for (var j = 0; j < 2; j++) { - var testView = new OCA.Files.DetailTabView('testtab' + j); + var testView = new OCA.Files.DetailTabView({id: 'testtab' + j}); testView.index = j; testView.getLabel = function() { return 'Test tab ' + this.index; }; testView.render = function() { @@ -119,26 +121,34 @@ } }, + template: function(vars) { + if (!this._template) { + this._template = Handlebars.compile(TEMPLATE); + } + return this._template(vars); + }, + /** * Renders this details view */ render: function() { - var self = this; - - if (!this._template) { - this._template = Handlebars.compile(TEMPLATE); - } - - if (!this._templateTabHeader) { - this._templateTabHeader = Handlebars.compile(TEMPLATE_TAB_HEADER); - } - - this.$el.html(this._template({ + var templateVars = { closeLabel: t('files', 'Close') - })); + }; + + if (this._tabViews.length > 1) { + // only render headers if there is more than one available + templateVars.tabHeaders = _.map(this._tabViews, function(tabView, i) { + return { + tabId: tabView.id, + tabIndex: i, + label: tabView.getLabel() + }; + }); + } + + this.$el.html(this.template(templateVars)); - var $tabsContainer = this.$el.find('.tabsContainer'); - var $tabHeadsContainer = this.$el.find('.tabHeaders'); var $detailsContainer = this.$el.find('.detailFileInfoContainer'); // render details @@ -146,29 +156,58 @@ $detailsContainer.append(detailView.get$()); }); - if (this._tabViews.length > 0) { - if (!this._currentTab) { - this._currentTab = this._tabViews[0].id; - } - - // render tabs - _.each(this._tabViews, function(tabView, i) { - // hidden by default - var $el = tabView.get$(); - var isCurrent = (tabView.id === self._currentTab); - if (!isCurrent) { - $el.addClass('hidden'); - } - $tabsContainer.append($el); - - $tabHeadsContainer.append(self._templateTabHeader({ - tabId: tabView.id, - tabIndex: i, - label: tabView.getLabel(), - selected: isCurrent - })); - }); + if (!this._currentTabId && this._tabViews.length > 0) { + this._currentTabId = this._tabViews[0].id; } + + this.selectTab(this._currentTabId); + + this._dirty = false; + }, + + /** + * Selects the given tab by id + * + * @param {string} tabId tab id + */ + selectTab: function(tabId) { + if (!tabId) { + return; + } + + var tabView = _.find(this._tabViews, function(tab) { + return tab.id === tabId; + }); + + if (!tabView) { + console.warn('Details view tab with id "' + tabId + '" not found'); + return; + } + + this._currentTabId = tabId; + + var $tabsContainer = this.$el.find('.tabsContainer'); + var $tabEl = $tabsContainer.find('#' + tabId); + + // hide other tabs + $tabsContainer.find('.tab').addClass('hidden'); + + // tab already rendered ? + if (!$tabEl.length) { + // render tab + $tabsContainer.append(tabView.$el); + $tabEl = tabView.$el; + } + + // this should trigger tab rendering + tabView.setFileInfo(this.model); + + $tabEl.removeClass('hidden'); + + // update tab headers + var $tabHeaders = this.$el.find('.tabHeaders li'); + $tabHeaders.removeClass('selected'); + $tabHeaders.filterAttr('data-tabid', tabView.id).addClass('selected'); }, /** @@ -179,12 +218,19 @@ setFileInfo: function(fileInfo) { this.model = fileInfo; - this.render(); + if (this._dirty) { + this.render(); + } - // notify all panels - _.each(this._tabViews, function(tabView) { + if (this._currentTabId) { + // only update current tab, others will be updated on-demand + var tabId = this._currentTabId; + var tabView = _.find(this._tabViews, function(tab) { + return tab.id === tabId; + }); tabView.setFileInfo(fileInfo); - }); + } + _.each(this._detailFileInfoViews, function(detailView) { detailView.setFileInfo(fileInfo); }); @@ -206,6 +252,7 @@ */ addTabView: function(tabView) { this._tabViews.push(tabView); + this._dirty = true; }, /** @@ -215,6 +262,7 @@ */ addDetailView: function(detailView) { this._detailFileInfoViews.push(detailView); + this._dirty = true; } }); diff --git a/apps/files/js/detailtabview.js b/apps/files/js/detailtabview.js index b2e02971fb4..67f8b535abd 100644 --- a/apps/files/js/detailtabview.js +++ b/apps/files/js/detailtabview.js @@ -71,8 +71,10 @@ * @param {OCA.Files.FileInfoModel} fileInfo file info to set */ setFileInfo: function(fileInfo) { - this.model = fileInfo; - this.render(); + if (this.model !== fileInfo) { + this.model = fileInfo; + this.render(); + } }, /** diff --git a/apps/files/js/tagsplugin.js b/apps/files/js/tagsplugin.js index d8552c71e45..ed1105a1706 100644 --- a/apps/files/js/tagsplugin.js +++ b/apps/files/js/tagsplugin.js @@ -108,6 +108,8 @@ } toggleStar($actionEl, !isFavorite); + context.fileInfoModel.set('tags', tags); + self.applyFileTags( dir + '/' + fileName, tags, @@ -124,7 +126,6 @@ toggleStar($actionEl, (newTags.indexOf(OC.TAG_FAVORITE) >= 0)); $file.attr('data-tags', newTags.join('|')); $file.attr('data-favorite', !isFavorite); - context.fileInfoModel.set('tags', newTags); fileInfo.tags = newTags; }); } diff --git a/apps/files/templates/test.png b/apps/files/templates/test.png new file mode 100644 index 0000000000000000000000000000000000000000..2b90216f797dbc275876f6334bb4a5c504ce3a85 GIT binary patch literal 2302 zcmV#pis-T$t>b?e@q z4uT+r{4gcV4@E&vc1nuE{4gctM>K^{D@@8^$&yS8wMwAokY*LD5JG+qM3yB#gef@; zS(Ygw%+F!Svdj;;8NU)dfgFTNkQ{;>)Ev|t)LoK#&NWqj4rlhDcCiv1lf#)UsF&EH zq}0#h%nsD{iX)_NRQQ;i-Mqs#t^7_9yl-f!xwTVY|H>h1=q|+*V%y84aWVj&csZEk?tX}uk6paY`ceKpe8Y4YOQn#Q2Op;1p~=%s2@*HV9beG^az zMo7ooqr%6`c7b7LNA`G;AXw;P!6@TQwnlY9ffWg;EloGl2hpu4^dHkKuu#ne)Nx*L zvta5wLkzWtP3D+mjypo){7pswD6 zpr?j<>lz)^+KI8f9wa|){y)Tdv61SjnV`Ntk>5}vEnK8=+*56RqB*~bK~9WDaV0>l zr%+~P?RPs4P?1LCGDN!EQMqO6_NA83GK6}1ZtcG#IU_@)f5`<@cHCrChEV%|!SduSYp9hM}Y#hRnftv zMmf{oWjs$??3cFpCXhGQ=bnr-A^KY$EbY25d3g&+@BIV2;5Cm0? zv^n0lNSCN>Kde;Wauf=JDsGIEMf^4q+}>rVEoE9>+iuAE$Our-9}=_l)mUStuS|8P z#fhoxAn+mP8tZxRNlIM4kyT)BO4Q*b<1NQhxoRx?uK^{F)Fx-@)O6 z?qF72b61LY(jJ_YJ*eI6O!;{^xm0+_2qTOz6Ji@YG|eUJ2yTg?-kNr6z9>U~eYZyz zv!G0dkn-FDqvjR2Gyo`~qE|u3Tkzr-Dy(|T4)41(k z;NBSOn1r4wC|149KH4b8yfUG#CdxT%OSx=&8cLvck83`z@IcwgAoWi&hI)B9nqEht zF|n+{t0_>Ah}-59+1wJScO>*IB+A5=CC_0~G#*o-zJoQf840eBAM13eui+{Cr$~+L zrc++c2g50EVVbKevE0W2j4F7$qLb%yQ3m9+pPGU%|gp`lD^U z{W54=2Gl=bQA&;=>p7}UWoDzwD}Bq+GB|dpw#n(>;sn%>HH(rXg;;8@7%Bth3Gz&a zD7muD?w=fId<(#%(e}eAioKDJiZLy^Jtx_MhlMhrc8)ydI+X`C-I3m2d7xY!&tpZ3 z6J5)CJePo4q{tKQi{2#c7iDrwYWTnebBT{h#?lsh)(oG6Rk2%jdE}@)8fCpo=KoiW zbdN9bd6fN4hZ@`uHhIcuGc0sXW#s**RtV`ki_ZksWr;+X0sD!{*FSeEXhI5LH%|>&As!VlN{6>)Nei1 z+`p!0TZ(cYQOLf`&wcJA`!Xf>VVdkqe#m`7Da$e`_pz_6BaJ@xCFDL!mxUQW@mae6 Y0^QUac<*?*PXGV_07*qoM6N<$g0sYB)Bpeg literal 0 HcmV?d00001 diff --git a/apps/files/tests/js/detailsviewSpec.js b/apps/files/tests/js/detailsviewSpec.js index 4261aa53c94..852f8b04293 100644 --- a/apps/files/tests/js/detailsviewSpec.js +++ b/apps/files/tests/js/detailsviewSpec.js @@ -67,32 +67,75 @@ describe('OCA.Files.DetailsView tests', function() { var testView, testView2; beforeEach(function() { - testView = new OCA.Files.DetailTabView('test1'); - testView2 = new OCA.Files.DetailTabView('test2'); + testView = new OCA.Files.DetailTabView({id: 'test1'}); + testView2 = new OCA.Files.DetailTabView({id: 'test2'}); detailsView.addTabView(testView); detailsView.addTabView(testView2); detailsView.render(); }); - it('renders registered tabs', function() { - expect(detailsView.$el.find('.tab').length).toEqual(2); + it('initially renders only the selected tab', function() { + expect(detailsView.$el.find('.tab').length).toEqual(1); + expect(detailsView.$el.find('.tab').attr('id')).toEqual('test1'); }); - it('updates registered tabs when fileinfo is updated', function() { - var tabRenderStub = sinon.stub(OCA.Files.DetailTabView.prototype, 'render'); - var fileInfo = {id: 5, name: 'test.txt'}; - tabRenderStub.reset(); - detailsView.setFileInfo(fileInfo); + it('updates tab model and rerenders on-demand as soon as it gets selected', function() { + var tab1RenderStub = sinon.stub(testView, 'render'); + var tab2RenderStub = sinon.stub(testView2, 'render'); + var fileInfo1 = new OCA.Files.FileInfoModel({id: 5, name: 'test.txt'}); + var fileInfo2 = new OCA.Files.FileInfoModel({id: 8, name: 'test2.txt'}); - expect(testView.getFileInfo()).toEqual(fileInfo); - expect(testView2.getFileInfo()).toEqual(fileInfo); + detailsView.setFileInfo(fileInfo1); - expect(tabRenderStub.callCount).toEqual(2); - tabRenderStub.restore(); + // first tab renders, not the second one + expect(tab1RenderStub.calledOnce).toEqual(true); + expect(tab2RenderStub.notCalled).toEqual(true); + + // info got set only to the first visible tab + expect(testView.getFileInfo()).toEqual(fileInfo1); + expect(testView2.getFileInfo()).toBeUndefined(); + + // select second tab for first render + detailsView.$el.find('.tabHeader').eq(1).click(); + + // second tab got rendered + expect(tab2RenderStub.calledOnce).toEqual(true); + expect(testView2.getFileInfo()).toEqual(fileInfo1); + + // select the first tab again + detailsView.$el.find('.tabHeader').eq(0).click(); + + // no re-render + expect(tab1RenderStub.calledOnce).toEqual(true); + expect(tab2RenderStub.calledOnce).toEqual(true); + + tab1RenderStub.reset(); + tab2RenderStub.reset(); + + // switch to another file + detailsView.setFileInfo(fileInfo2); + + // only the visible tab was updated and rerendered + expect(tab1RenderStub.calledOnce).toEqual(true); + expect(testView.getFileInfo()).toEqual(fileInfo2); + + // second/invisible tab still has old info, not rerendered + expect(tab2RenderStub.notCalled).toEqual(true); + expect(testView2.getFileInfo()).toEqual(fileInfo1); + + // reselect the second one + detailsView.$el.find('.tabHeader').eq(1).click(); + + // second tab becomes visible, updated and rendered + expect(testView2.getFileInfo()).toEqual(fileInfo2); + expect(tab2RenderStub.calledOnce).toEqual(true); + + tab1RenderStub.restore(); + tab2RenderStub.restore(); }); it('selects the first tab by default', function() { expect(detailsView.$el.find('.tabHeader').eq(0).hasClass('selected')).toEqual(true); expect(detailsView.$el.find('.tabHeader').eq(1).hasClass('selected')).toEqual(false); expect(detailsView.$el.find('.tab').eq(0).hasClass('hidden')).toEqual(false); - expect(detailsView.$el.find('.tab').eq(1).hasClass('hidden')).toEqual(true); + expect(detailsView.$el.find('.tab').eq(1).length).toEqual(0); }); it('switches the current tab when clicking on tab header', function() { detailsView.$el.find('.tabHeader').eq(1).click(); @@ -101,5 +144,14 @@ describe('OCA.Files.DetailsView tests', function() { expect(detailsView.$el.find('.tab').eq(0).hasClass('hidden')).toEqual(true); expect(detailsView.$el.find('.tab').eq(1).hasClass('hidden')).toEqual(false); }); + it('does not render tab headers when only one tab exists', function() { + detailsView.remove(); + detailsView = new OCA.Files.DetailsView(); + testView = new OCA.Files.DetailTabView({id: 'test1'}); + detailsView.addTabView(testView); + detailsView.render(); + + expect(detailsView.$el.find('.tabHeader').length).toEqual(0); + }); }); }); diff --git a/apps/files_sharing/js/share.js b/apps/files_sharing/js/share.js index 04700b84011..c124d390d04 100644 --- a/apps/files_sharing/js/share.js +++ b/apps/files_sharing/js/share.js @@ -60,6 +60,14 @@ return tr; }; + var oldElementToFile = fileList.elementToFile; + fileList.elementToFile = function($el) { + var fileInfo = oldElementToFile.apply(this, arguments); + fileInfo.sharePermissions = $el.attr('data-share-permissions') || undefined; + fileInfo.shareOwner = $el.attr('data-share-owner') || undefined; + return fileInfo; + }; + // use delegate to catch the case with multiple file lists fileList.$el.on('fileActionsReady', function(ev){ var fileList = ev.fileList; diff --git a/apps/files_sharing/js/sharetabview.js b/apps/files_sharing/js/sharetabview.js index 5f4a21a4a57..ee572b747ea 100644 --- a/apps/files_sharing/js/sharetabview.js +++ b/apps/files_sharing/js/sharetabview.js @@ -10,7 +10,7 @@ (function() { var TEMPLATE = - '
    Owner: {{owner}}'; + '
      {{#if owner}}
    • Owner: {{owner}}
      • {{/if}}
    '; /** * @memberof OCA.Sharing @@ -37,8 +37,13 @@ } if (this.model) { + console.log(this.model); + var owner = this.model.get('shareOwner'); + if (owner === OC.currentUser) { + owner = null; + } this.$el.append(this._template({ - owner: this.model.get('shareOwner') || OC.currentUser + owner: owner })); } else {