Skip to content

Commit 9986d67

Browse files
hectahertzfrancinelucca
hectahertz
and
francinelucca
authoredJan 16, 2025··
Pagination algorithm enhancements (#5504)
* Simplify Pagination algorithm with an early return * Pagination: Keep a consistent width on next and previous * Simplify Pagination algorithm and keep number of pages shown consistent * Add changeset * Replicate style changes to Pagination.module.css * wip: algo refactor Co-authored-by: Hector Garcia <[email protected]> * refactor(Pagination): refine algorithm * fix: lint * pagination: small tweaks * fix(Pagination): refine logic, add tests * chore: remove outdated comments * test(vrt): update snapshots * test(vrt): update snapshots * chore(Pagination): refactoor Co-authored-by: Hector Garcia <[email protected]> * fix(Pagination): remove unused type --------- Co-authored-by: Marie Lucca <[email protected]> Co-authored-by: Hector Garcia <[email protected]> Co-authored-by: Marie Lucca <[email protected]> Co-authored-by: francinelucca <[email protected]>
·
@primer/react@37.18.0@primer/react@37.11.0
1 parent 49bf6f1 commit 9986d67

23 files changed

+325
-138
lines changed
 

‎.changeset/serious-melons-own.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@primer/react": patch
3+
---
4+
5+
Pagination: Optimize the page rendering algorithm and prevent layout shifts.

‎.playwright/snapshots/components/Pagination.test.ts-snapshots/Pagehead-Default-dark-linux.png

185 Bytes
Loading

‎.playwright/snapshots/components/Pagination.test.ts-snapshots/Pagehead-Default-light-linux.png

184 Bytes
Loading

‎packages/react/src/Pagination/Pagination.module.css

+2-10
Original file line numberDiff line numberDiff line change
@@ -103,16 +103,8 @@
103103
box-shadow: inset 0 0 0 3px var(--fgColor-onEmphasis);
104104
}
105105

106-
.Page[aria-disabled]:first-child,
107-
.Page[aria-disabled]:hover:first-child {
108-
/* stylelint-disable-next-line primer/spacing */
109-
margin: 0 2px;
110-
/* stylelint-disable-next-line primer/spacing */
111-
margin-right: 6px;
112-
}
113-
114-
.Page[aria-disabled],
115-
.Page[aria-disabled]:hover,
106+
.Page[aria-hidden],
107+
.Page[aria-hidden]:hover,
116108
.Page[role='presentation'],
117109
.Page[role='presentation']:hover {
118110
color: var(--fgColor-disabled);

‎packages/react/src/Pagination/Pagination.tsx

+2-11
Original file line numberDiff line numberDiff line change
@@ -91,17 +91,8 @@ const Page = toggleStyledComponent(
9191
box-shadow: inset 0 0 0 3px ${get('colors.fg.onEmphasis')};
9292
}
9393
94-
&[aria-disabled],
95-
&[aria-disabled]:hover {
96-
margin: 0 2px;
97-
98-
&:first-child {
99-
margin-right: 6px;
100-
}
101-
}
102-
103-
&[aria-disabled],
104-
&[aria-disabled]:hover,
94+
&[aria-hidden],
95+
&[aria-hidden]:hover,
10596
&[role='presentation'],
10697
&[role='presentation']:hover {
10798
color: ${get('colors.primer.fg.disabled')}; // check

‎packages/react/src/Pagination/model.tsx

+112-111
Original file line numberDiff line numberDiff line change
@@ -5,125 +5,126 @@ export function buildPaginationModel(
55
marginPageCount: number,
66
surroundingPageCount: number,
77
) {
8-
const pages = []
8+
const prev: PageType = {type: 'PREV', num: currentPage - 1, disabled: currentPage === 1}
9+
const next: PageType = {type: 'NEXT', num: currentPage + 1, disabled: currentPage === pageCount}
10+
if (!showPages) {
11+
return [prev, next]
12+
}
913

10-
if (showPages) {
11-
const pageNums: Array<number> = []
12-
const addPage = (n: number) => {
13-
if (n >= 1 && n <= pageCount) {
14-
pageNums.push(n)
15-
}
16-
}
14+
if (pageCount <= 0) {
15+
return [prev, {...next, disabled: true}]
16+
}
1717

18-
// Start by defining the window of pages to show around the current page.
19-
// If the window goes off either edge, shift it until it fits.
20-
let extentLeft = currentPage - surroundingPageCount
21-
let extentRight = currentPage + surroundingPageCount
22-
if (extentLeft < 1 && extentRight > pageCount) {
23-
// Our window is larger than the entire range,
24-
// so simply display every page.
25-
extentLeft = 1
26-
extentRight = pageCount
27-
} else if (extentLeft < 1) {
28-
while (extentLeft < 1) {
29-
extentLeft++
30-
extentRight++
31-
}
32-
} else if (extentRight > pageCount) {
33-
while (extentRight > pageCount) {
34-
extentLeft--
35-
extentRight--
36-
}
37-
}
18+
const pages: PageType[] = []
3819

39-
// Next, include the pages in the margins.
40-
// If a margin page is already covered in the window,
41-
// extend the window to the other direction.
42-
for (let i = 1; i <= marginPageCount; i++) {
43-
const leftPage = i
44-
const rightPage = pageCount - (i - 1)
45-
if (leftPage >= extentLeft) {
46-
extentRight++
47-
} else {
48-
addPage(leftPage)
49-
}
50-
if (rightPage <= extentRight) {
51-
extentLeft--
52-
} else {
53-
addPage(rightPage)
54-
}
55-
}
20+
// number of pages shown on each side of the current page
21+
// [1, ..., 7, 8, _9_, 10, 11, ..., 15]
22+
// standardGap: 3
23+
const standardGap = surroundingPageCount + marginPageCount
5624

57-
for (let i = extentLeft; i <= extentRight; i++) {
58-
addPage(i)
59-
}
25+
// the maximum number of pages that can be shown at a given time
26+
// [1, ..., 7, 8, _9_, 10, 11, ..., 15]
27+
// maxVisiblePages: 7
28+
const maxVisiblePages = standardGap + standardGap + 1
6029

61-
const sorted = pageNums
62-
.slice()
63-
.sort((a, b) => a - b)
64-
.filter((item, idx, ary) => !idx || item !== ary[idx - 1])
65-
for (let idx = 0; idx < sorted.length; idx++) {
66-
const num = sorted[idx]
67-
const selected = num === currentPage
68-
const last = sorted[idx - 1]
69-
const next = sorted[idx + 1]
70-
const lastDelta = num - last
71-
const nextDelta = num - next
72-
const precedesBreak = nextDelta !== -1
73-
74-
if (idx === 0) {
75-
if (num !== 1) {
76-
// If the first page isn't page one,
77-
// we need to add a break
78-
pages.push({
79-
type: 'BREAK',
80-
num: 1,
81-
})
82-
}
83-
pages.push({
84-
type: 'NUM',
85-
num,
86-
selected,
87-
precedesBreak,
88-
})
89-
} else {
90-
if (lastDelta === 1) {
91-
pages.push({
92-
type: 'NUM',
93-
num,
94-
selected,
95-
precedesBreak,
96-
})
97-
} else {
98-
// We skipped some, so add a break
99-
pages.push({
100-
type: 'BREAK',
101-
num: num - 1,
102-
})
103-
pages.push({
104-
type: 'NUM',
105-
num,
106-
selected,
107-
precedesBreak: false,
108-
})
109-
}
110-
}
111-
}
30+
// if the number of pages is less than the maximum number of pages that can be shown just return all of them
31+
if (pageCount <= maxVisiblePages) {
32+
addPages(1, pageCount, false)
33+
return [prev, ...pages, next]
34+
}
35+
36+
// startGap is the number of pages hidden by the start ellipsis
37+
// startOffset is the number of pages to offset at the start to compensate
38+
// [1, ..., 7, 8, _9_, 10, 11, ..., 15]
39+
// startGap: 5
40+
// startOffset: 0
41+
// when the margin and the surrounding windows overlap.
42+
// [1, _2_, 3, 4, 5, 6, ..., 15]
43+
// startGap = 0
44+
// startOffset: -3 <--
45+
let startGap = 0
46+
let startOffset = 0
11247

113-
const lastPage = pages[pages.length - 1]
114-
if (lastPage.type === 'NUM' && lastPage.num !== pageCount) {
115-
// The last page we rendered wasn't the actual last page,
116-
// so we need an additional break
48+
// When there is overlap
49+
if (currentPage - standardGap - 1 <= 1) {
50+
startOffset = currentPage - standardGap - 2
51+
} else {
52+
startGap = currentPage - standardGap - 1
53+
}
54+
55+
// These are equivalent to startGap and startOffset but at the end of the list
56+
let endGap = 0
57+
let endOffset = 0
58+
59+
// When there is overlap
60+
if (pageCount - currentPage - standardGap <= 1) {
61+
endOffset = pageCount - currentPage - standardGap - 1
62+
} else {
63+
endGap = pageCount - currentPage - standardGap
64+
}
65+
66+
const hasStartEllipsis = startGap > 0
67+
const hasEndEllipsis = endGap > 0
68+
69+
// add pages "before" the start ellipsis (if any)
70+
// [1, ..., 7, 8, _9_, 10, 11, ..., 15]
71+
// marginPageCount: 1
72+
// addPages(1, 1, true)
73+
addPages(1, marginPageCount, hasStartEllipsis)
74+
75+
if (hasStartEllipsis) {
76+
addEllipsis(marginPageCount)
77+
}
78+
79+
// add middle pages
80+
// [1, ..., 7, 8, _9_, 10, 11, ..., 15]
81+
// marginPageCount: 1
82+
// surroundingPageCount: 2
83+
// startGap: 5
84+
// startOffset: 0
85+
// endGap: 3
86+
// endOffset: 0
87+
// addPages(7, 11, true)
88+
addPages(
89+
marginPageCount + startGap + endOffset + 1,
90+
pageCount - startOffset - endGap - marginPageCount,
91+
hasEndEllipsis,
92+
)
93+
94+
if (hasEndEllipsis) {
95+
addEllipsis(pageCount - startOffset - endGap - marginPageCount)
96+
}
97+
98+
// add pages "after" the start ellipsis (if any)
99+
// [1, ..., 7, 8, _9_, 10, 11, ..., 15]
100+
// marginPageCount: 1
101+
// surroundingPageCount: 2
102+
// startGap: 5
103+
// startOffset: 0
104+
// endGap: 3
105+
// endOffset: 0
106+
// addPages(15, 15)
107+
addPages(pageCount - marginPageCount + 1, pageCount)
108+
109+
return [prev, ...pages, next]
110+
111+
function addEllipsis(previousPage: number): void {
112+
pages.push({
113+
type: 'BREAK',
114+
num: previousPage + 1,
115+
})
116+
}
117+
118+
function addPages(start: number, end: number, precedesBreak: boolean = false): void {
119+
for (let i = start; i <= end; i++) {
117120
pages.push({
118-
type: 'BREAK',
119-
num: pageCount,
121+
type: 'NUM',
122+
num: i,
123+
selected: i === currentPage,
124+
precedesBreak: i === end && precedesBreak,
120125
})
121126
}
122127
}
123-
124-
const prev = {type: 'PREV', num: currentPage - 1, disabled: currentPage === 1}
125-
const next = {type: 'NEXT', num: currentPage + 1, disabled: currentPage === pageCount}
126-
return [prev, ...pages, next]
127128
}
128129

129130
type PageType = {
@@ -148,7 +149,7 @@ export function buildComponentData(
148149
key = 'page-prev'
149150
content = 'Previous'
150151
if (page.disabled) {
151-
Object.assign(props, {'aria-hidden': 'true'})
152+
Object.assign(props, {rel: 'prev', 'aria-hidden': 'true'})
152153
} else {
153154
Object.assign(props, {
154155
rel: 'prev',
@@ -163,7 +164,7 @@ export function buildComponentData(
163164
key = 'page-next'
164165
content = 'Next'
165166
if (page.disabled) {
166-
Object.assign(props, {'aria-hidden': 'true'})
167+
Object.assign(props, {rel: 'next', 'aria-hidden': 'true'})
167168
} else {
168169
Object.assign(props, {
169170
rel: 'next',

‎packages/react/src/__tests__/Pagination/PaginationModel.test.tsx

+204-6
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,170 @@ function last(array: Array<any>, count = 1) {
1414
}
1515

1616
describe('Pagination model', () => {
17+
it('correctly handles negative pages', () => {
18+
const model = buildPaginationModel(-10, 1, true, 1, 2)
19+
expect(first(model).type).toEqual('PREV')
20+
expect(first(model).disabled).toBe(true)
21+
expect(last(model).type).toEqual('NEXT')
22+
expect(last(model).disabled).toBe(true)
23+
expect(model.length).toBe(2)
24+
})
25+
26+
it('correctly handles zero pages', () => {
27+
const model = buildPaginationModel(0, 1, true, 1, 2)
28+
expect(first(model).type).toEqual('PREV')
29+
expect(first(model).disabled).toBe(true)
30+
expect(last(model).type).toEqual('NEXT')
31+
expect(last(model).disabled).toBe(true)
32+
expect(model.length).toBe(2)
33+
})
34+
35+
it('correctly handles 1 page', () => {
36+
const model = buildPaginationModel(1, 1, true, 1, 2)
37+
expect(first(model).type).toEqual('PREV')
38+
expect(first(model).disabled).toBe(true)
39+
expect(last(model).type).toEqual('NEXT')
40+
expect(last(model).disabled).toBe(true)
41+
expect(model.length).toBe(3)
42+
})
43+
44+
it('correctly handles zero margin pages', () => {
45+
const model = buildPaginationModel(6, 2, true, 0, 2)
46+
47+
const expected = [
48+
{
49+
type: 'PREV',
50+
num: 1,
51+
disabled: false,
52+
},
53+
{
54+
type: 'NUM',
55+
num: 1,
56+
selected: false,
57+
precedesBreak: false,
58+
},
59+
{
60+
type: 'NUM',
61+
num: 2,
62+
selected: true,
63+
precedesBreak: false,
64+
},
65+
{
66+
type: 'NUM',
67+
num: 3,
68+
selected: false,
69+
precedesBreak: false,
70+
},
71+
{
72+
type: 'NUM',
73+
num: 4,
74+
selected: false,
75+
precedesBreak: false,
76+
},
77+
{
78+
type: 'NUM',
79+
num: 5,
80+
selected: false,
81+
precedesBreak: false,
82+
},
83+
{
84+
type: 'NUM',
85+
num: 6,
86+
selected: false,
87+
precedesBreak: true,
88+
},
89+
{
90+
type: 'BREAK',
91+
num: 7,
92+
},
93+
{
94+
type: 'NEXT',
95+
num: 3,
96+
disabled: false,
97+
},
98+
]
99+
100+
expect(model).toMatchObject(expected)
101+
})
102+
103+
it('correctly handles zero surrounding pages', () => {
104+
const model = buildPaginationModel(7, 4, true, 1, 0)
105+
106+
const expected = [
107+
{
108+
type: 'PREV',
109+
num: 3,
110+
disabled: false,
111+
},
112+
{
113+
type: 'NUM',
114+
num: 1,
115+
selected: false,
116+
precedesBreak: true,
117+
},
118+
{
119+
type: 'BREAK',
120+
num: 2,
121+
},
122+
{
123+
type: 'NUM',
124+
num: 4,
125+
selected: true,
126+
precedesBreak: true,
127+
},
128+
{
129+
type: 'BREAK',
130+
num: 5,
131+
},
132+
{
133+
type: 'NUM',
134+
num: 7,
135+
selected: false,
136+
precedesBreak: false,
137+
},
138+
{
139+
type: 'NEXT',
140+
num: 5,
141+
disabled: false,
142+
},
143+
]
144+
145+
expect(model).toMatchObject(expected)
146+
})
147+
148+
it('correctly handles zero margin and surrounding pages', () => {
149+
const model = buildPaginationModel(50, 3, true, 0, 0)
150+
151+
const expected = [
152+
{
153+
type: 'PREV',
154+
num: 2,
155+
disabled: false,
156+
},
157+
{
158+
type: 'BREAK',
159+
num: 1,
160+
},
161+
{
162+
type: 'NUM',
163+
num: 3,
164+
selected: true,
165+
precedesBreak: true,
166+
},
167+
{
168+
type: 'BREAK',
169+
num: 4,
170+
},
171+
{
172+
type: 'NEXT',
173+
num: 4,
174+
disabled: false,
175+
},
176+
]
177+
178+
expect(model).toMatchObject(expected)
179+
})
180+
17181
it('sets disabled on prev links', () => {
18182
const model1 = buildPaginationModel(10, 1, true, 1, 2)
19183
expect(first(model1).type).toEqual('PREV')
@@ -94,26 +258,60 @@ describe('Pagination model', () => {
94258
{type: 'NUM', num: 2, selected: true},
95259
{type: 'NUM', num: 3},
96260
// normally with a surround of 1, only 1 and 3 would be shown
97-
// however, since 1 was already shown, we extend to 4
98-
{type: 'NUM', num: 4, precedesBreak: true},
261+
// however, since we don't overlap, the window is extended to 5
262+
{type: 'NUM', num: 4},
263+
{type: 'NUM', num: 5, precedesBreak: true},
99264
{type: 'BREAK'},
100265
]
101-
expect(first(model, 6)).toMatchObject(expected)
266+
expect(first(model, 7)).toMatchObject(expected)
102267
})
103268

104269
it('adds items to the left if it hits bounds to the right', () => {
105270
const model = buildPaginationModel(15, 14, true, 1, 1)
106271
const expected = [
107272
// normally with a surround of 1, only 13 and 15 would be shown
108-
// however, since 15 was already shown, we extend to 12
273+
// however, since we don't overlap, the window is extended to 11
109274
{type: 'BREAK'},
275+
{type: 'NUM', num: 11},
110276
{type: 'NUM', num: 12},
111277
{type: 'NUM', num: 13},
112278
{type: 'NUM', num: 14, selected: true},
113279
{type: 'NUM', num: 15},
114280
{type: 'NEXT', num: 15},
115281
]
116-
expect(last(model, 6)).toMatchObject(expected)
282+
expect(last(model, 7)).toMatchObject(expected)
283+
})
284+
285+
it('adds a page when there would be only one page hidden by the left ellipsis', () => {
286+
const model = buildPaginationModel(15, 5, true, 1, 2)
287+
const expected = [
288+
{type: 'PREV', num: 4},
289+
{type: 'NUM', num: 1},
290+
{type: 'NUM', num: 2},
291+
{type: 'NUM', num: 3},
292+
{type: 'NUM', num: 4},
293+
{type: 'NUM', num: 5, selected: true},
294+
{type: 'NUM', num: 6},
295+
{type: 'NUM', num: 7, precedesBreak: true},
296+
{type: 'BREAK'},
297+
]
298+
expect(first(model, 9)).toMatchObject(expected)
299+
})
300+
301+
it('adds a page when there would be only one page hidden by the right ellipsis', () => {
302+
const model = buildPaginationModel(15, 11, true, 1, 2)
303+
const expected = [
304+
{type: 'BREAK'},
305+
{type: 'NUM', num: 9},
306+
{type: 'NUM', num: 10},
307+
{type: 'NUM', num: 11, selected: true},
308+
{type: 'NUM', num: 12},
309+
{type: 'NUM', num: 13},
310+
{type: 'NUM', num: 14},
311+
{type: 'NUM', num: 15},
312+
{type: 'NEXT', num: 12},
313+
]
314+
expect(last(model, 9)).toMatchObject(expected)
117315
})
118316

119317
it('correctly creates breaks next to the next/prev links when margin is 0', () => {
@@ -124,7 +322,7 @@ describe('Pagination model', () => {
124322
{type: 'NUM', num: 4},
125323
{type: 'NUM', num: 5, selected: true},
126324
{type: 'NUM', num: 6, precedesBreak: true},
127-
{type: 'BREAK', num: 10},
325+
{type: 'BREAK', num: 7},
128326
{type: 'NEXT'},
129327
]
130328
expect(model).toMatchObject(expected)

0 commit comments

Comments
 (0)
Please sign in to comment.