From d9dd52092ff7e489c162ddf436fb496cc8144e73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 27 Feb 2019 14:32:08 +0100 Subject: Make JS pagination handle missing 'X-Total-Pages' header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .../javascripts/pipelines/mixins/pipelines.js | 6 +- .../vue_shared/components/table_pagination.vue | 33 ++-- ...here-the-x-total-pages-header-isn-t-present.yml | 5 + .../vue_shared/components/table_pagination_spec.js | 182 +++++++++++++++++++-- 4 files changed, 189 insertions(+), 37 deletions(-) create mode 100644 changelogs/unreleased/57991-frontend-pagination-needs-to-handle-cases-where-the-x-total-pages-header-isn-t-present.yml diff --git a/app/assets/javascripts/pipelines/mixins/pipelines.js b/app/assets/javascripts/pipelines/mixins/pipelines.js index 74ca3071364..3cc9d0a3a4e 100644 --- a/app/assets/javascripts/pipelines/mixins/pipelines.js +++ b/app/assets/javascripts/pipelines/mixins/pipelines.js @@ -27,11 +27,7 @@ export default { }, computed: { shouldRenderPagination() { - return ( - !this.isLoading && - this.state.pipelines.length && - this.state.pageInfo.total > this.state.pageInfo.perPage - ); + return !this.isLoading; }, }, beforeMount() { diff --git a/app/assets/javascripts/vue_shared/components/table_pagination.vue b/app/assets/javascripts/vue_shared/components/table_pagination.vue index 9f38c2e4b9e..8e0b08032f7 100644 --- a/app/assets/javascripts/vue_shared/components/table_pagination.vue +++ b/app/assets/javascripts/vue_shared/components/table_pagination.vue @@ -54,15 +54,14 @@ export default { return this.pageInfo.nextPage; }, getItems() { - const total = this.pageInfo.totalPages; - const { page } = this.pageInfo; + const { totalPages, nextPage, previousPage, page } = this.pageInfo; const items = []; if (page > 1) { items.push({ title: FIRST, first: true }); } - if (page > 1) { + if (previousPage) { items.push({ title: PREV, prev: true }); } else { items.push({ title: PREV, disabled: true, prev: true }); @@ -70,32 +69,34 @@ export default { if (page > UI_LIMIT) items.push({ title: SPREAD, separator: true }); - const start = Math.max(page - PAGINATION_UI_BUTTON_LIMIT, 1); - const end = Math.min(page + PAGINATION_UI_BUTTON_LIMIT, total); + if (totalPages) { + const start = Math.max(page - PAGINATION_UI_BUTTON_LIMIT, 1); + const end = Math.min(page + PAGINATION_UI_BUTTON_LIMIT, totalPages); - for (let i = start; i <= end; i += 1) { - const isActive = i === page; - items.push({ title: i, active: isActive, page: true }); - } + for (let i = start; i <= end; i += 1) { + const isActive = i === page; + items.push({ title: i, active: isActive, page: true }); + } - if (total - page > PAGINATION_UI_BUTTON_LIMIT) { - items.push({ title: SPREAD, separator: true, page: true }); + if (totalPages - page > PAGINATION_UI_BUTTON_LIMIT) { + items.push({ title: SPREAD, separator: true, page: true }); + } } - if (page === total) { - items.push({ title: NEXT, disabled: true, next: true }); - } else if (total - page >= 1) { + if (nextPage) { items.push({ title: NEXT, next: true }); + } else { + items.push({ title: NEXT, disabled: true, next: true }); } - if (total - page >= 1) { + if (totalPages && totalPages - page >= 1) { items.push({ title: LAST, last: true }); } return items; }, showPagination() { - return this.pageInfo.totalPages > 1; + return this.pageInfo.nextPage || this.pageInfo.previousPage; }, }, methods: { diff --git a/changelogs/unreleased/57991-frontend-pagination-needs-to-handle-cases-where-the-x-total-pages-header-isn-t-present.yml b/changelogs/unreleased/57991-frontend-pagination-needs-to-handle-cases-where-the-x-total-pages-header-isn-t-present.yml new file mode 100644 index 00000000000..2e18377a4cf --- /dev/null +++ b/changelogs/unreleased/57991-frontend-pagination-needs-to-handle-cases-where-the-x-total-pages-header-isn-t-present.yml @@ -0,0 +1,5 @@ +--- +title: "Improve the JS pagination to handle the case when the `X-Total` and `X-Total-Pages` headers aren't present" +merge_request: 25601 +author: +type: fixed diff --git a/spec/javascripts/vue_shared/components/table_pagination_spec.js b/spec/javascripts/vue_shared/components/table_pagination_spec.js index 407d1d59f83..42cd41381dc 100644 --- a/spec/javascripts/vue_shared/components/table_pagination_spec.js +++ b/spec/javascripts/vue_shared/components/table_pagination_spec.js @@ -22,10 +22,10 @@ describe('Pagination component', () => { it('should not render anything', () => { component = mountComponent({ pageInfo: { - nextPage: 1, + nextPage: NaN, page: 1, perPage: 20, - previousPage: null, + previousPage: NaN, total: 15, totalPages: 1, }, @@ -58,6 +58,28 @@ describe('Pagination component', () => { expect(spy).not.toHaveBeenCalled(); }); + it('should be disabled and non clickable when total and totalPages are NaN', () => { + component = mountComponent({ + pageInfo: { + nextPage: 2, + page: 1, + perPage: 20, + previousPage: NaN, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + expect( + component.$el.querySelector('.js-previous-button').classList.contains('disabled'), + ).toEqual(true); + + component.$el.querySelector('.js-previous-button .page-link').click(); + + expect(spy).not.toHaveBeenCalled(); + }); + it('should be enabled and clickable', () => { component = mountComponent({ pageInfo: { @@ -75,6 +97,24 @@ describe('Pagination component', () => { expect(spy).toHaveBeenCalledWith(1); }); + + it('should be enabled and clickable when total and totalPages are NaN', () => { + component = mountComponent({ + pageInfo: { + nextPage: 3, + page: 2, + perPage: 20, + previousPage: 1, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + component.$el.querySelector('.js-previous-button .page-link').click(); + + expect(spy).toHaveBeenCalledWith(1); + }); }); describe('first button', () => { @@ -99,6 +139,28 @@ describe('Pagination component', () => { expect(spy).toHaveBeenCalledWith(1); }); + + it('should call the change callback with the first page when total and totalPages are NaN', () => { + component = mountComponent({ + pageInfo: { + nextPage: 3, + page: 2, + perPage: 20, + previousPage: 1, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + const button = component.$el.querySelector('.js-first-button .page-link'); + + expect(button.textContent.trim()).toEqual('« First'); + + button.click(); + + expect(spy).toHaveBeenCalledWith(1); + }); }); describe('last button', () => { @@ -123,16 +185,32 @@ describe('Pagination component', () => { expect(spy).toHaveBeenCalledWith(5); }); + + it('should not render', () => { + component = mountComponent({ + pageInfo: { + nextPage: 3, + page: 2, + perPage: 20, + previousPage: 1, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + expect(component.$el.querySelector('.js-last-button .page-link')).toBeNull(); + }); }); describe('next button', () => { it('should be disabled and non clickable', () => { component = mountComponent({ pageInfo: { - nextPage: 5, + nextPage: NaN, page: 5, perPage: 20, - previousPage: 1, + previousPage: 4, total: 84, totalPages: 5, }, @@ -146,6 +224,26 @@ describe('Pagination component', () => { expect(spy).not.toHaveBeenCalled(); }); + it('should be disabled and non clickable when total and totalPages are NaN', () => { + component = mountComponent({ + pageInfo: { + nextPage: NaN, + page: 5, + perPage: 20, + previousPage: 4, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + expect(component.$el.querySelector('.js-next-button').textContent.trim()).toEqual('Next'); + + component.$el.querySelector('.js-next-button .page-link').click(); + + expect(spy).not.toHaveBeenCalled(); + }); + it('should be enabled and clickable', () => { component = mountComponent({ pageInfo: { @@ -163,6 +261,24 @@ describe('Pagination component', () => { expect(spy).toHaveBeenCalledWith(4); }); + + it('should be enabled and clickable when total and totalPages are NaN', () => { + component = mountComponent({ + pageInfo: { + nextPage: 4, + page: 3, + perPage: 20, + previousPage: 2, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + component.$el.querySelector('.js-next-button .page-link').click(); + + expect(spy).toHaveBeenCalledWith(4); + }); }); describe('numbered buttons', () => { @@ -181,22 +297,56 @@ describe('Pagination component', () => { expect(component.$el.querySelectorAll('.page').length).toEqual(5); }); + + it('should not render any page', () => { + component = mountComponent({ + pageInfo: { + nextPage: 4, + page: 3, + perPage: 20, + previousPage: 2, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + expect(component.$el.querySelectorAll('.page').length).toEqual(0); + }); }); - it('should render the spread operator', () => { - component = mountComponent({ - pageInfo: { - nextPage: 4, - page: 3, - perPage: 20, - previousPage: 2, - total: 84, - totalPages: 10, - }, - change: spy, + describe('spread operator', () => { + it('should render', () => { + component = mountComponent({ + pageInfo: { + nextPage: 4, + page: 3, + perPage: 20, + previousPage: 2, + total: 84, + totalPages: 10, + }, + change: spy, + }); + + expect(component.$el.querySelector('.separator').textContent.trim()).toEqual('...'); }); - expect(component.$el.querySelector('.separator').textContent.trim()).toEqual('...'); + it('should not render', () => { + component = mountComponent({ + pageInfo: { + nextPage: 4, + page: 3, + perPage: 20, + previousPage: 2, + total: NaN, + totalPages: NaN, + }, + change: spy, + }); + + expect(component.$el.querySelector('.separator')).toBeNull(); + }); }); }); }); -- cgit v1.2.3