-
Notifications
You must be signed in to change notification settings - Fork 21
feat(Pagination): add Pagination component aligned with Cyber skin #1555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 18 commits
a3127a8
6d5331f
08e0990
aa4aada
d30e104
a9064f8
ad0df1c
c5c4061
8e9de99
f68dd1f
588bf02
ba5d146
7a4f114
857005b
b932a73
c4026f3
cedf287
fbb08ae
b3f213c
337bb8b
7d7f4de
604be41
cbc31aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import {openStoryPage, screen} from '../test-utils'; | ||
|
|
||
| import type {Device} from '../test-utils'; | ||
|
|
||
| const STORIES = [ | ||
| {id: 'components-pagination--default', name: 'Default'}, | ||
| {id: 'components-pagination--first-page', name: 'FirstPage'}, | ||
| {id: 'components-pagination--last-page', name: 'LastPage'}, | ||
| {id: 'components-pagination--with-ellipsis', name: 'WithEllipsis'}, | ||
| {id: 'components-pagination--nav-only-responsive', name: 'NavOnlyResponsive'}, | ||
| {id: 'components-pagination--pages-only', name: 'PagesOnly'}, | ||
| {id: 'components-pagination--icon-only-controls', name: 'IconOnlyControls'}, | ||
| {id: 'components-pagination--next-chapter-link', name: 'NextChapterLink'}, | ||
| ]; | ||
| const DEVICES = ['DESKTOP', 'MOBILE_IOS'] as const; | ||
|
|
||
| const getCases = () => { | ||
| const cases: Array<[string, string, Device]> = []; | ||
| for (const story of STORIES) { | ||
| for (const device of DEVICES) { | ||
| cases.push([story.name, story.id, device]); | ||
| } | ||
| } | ||
| return cases; | ||
| }; | ||
|
|
||
| test.each(getCases())('Pagination %s - %s', async (_name, id, device) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a table? |
||
| await openStoryPage({id, device}); | ||
|
|
||
| const pagination = await screen.findByTestId('Pagination'); | ||
| const image = await pagination.screenshot(); | ||
| expect(image).toMatchImageSnapshot(); | ||
| }); | ||
|
|
||
| // Compact view only renders below 375px viewport. We use MOBILE_IOS_SMALL (320px) | ||
| // to drive the vertical layout and the current ± 1 page reduction. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a lot of comments that we probably don't need, right? We should probably update AGENTS.md so it doesn't generate that much comments
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could take advantage of a feedback loop by using a skill that "auto documents" (better said: it helps updating/creating) things that the AI does not the way we want. each time we say "no", the skill p.d. |
||
| test('Pagination CompactView - MOBILE_IOS_SMALL', async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we sure we want this? by fiddling with the component, I see that even smaller viewports could take advantage of a single line, maybe. should we talk to design maybe?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aweell @AnaMontes11 what do you think? |
||
| await openStoryPage({ | ||
| id: 'components-pagination--compact-view', | ||
| device: 'MOBILE_IOS_SMALL', | ||
| }); | ||
|
|
||
| const pagination = await screen.findByTestId('Pagination'); | ||
| const image = await pagination.screenshot(); | ||
| expect(image).toMatchImageSnapshot(); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have added controls instead of creating a lot of examples like the rest of the stories. Perhaps this is something we should add to AGENTS.md so it is the default behaviour
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 100% agreed. we can improve this by exploiting controls. that way it will be improving the DX!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that some props are not exposed on storybook right? I'd cover |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import * as React from 'react'; | ||
| import {Pagination} from '..'; | ||
|
|
||
| export default { | ||
| title: 'Components/Pagination', | ||
| }; | ||
|
|
||
| /** | ||
| * Mid-range page with both navigation controls. On desktop renders | ||
| * "Previous 1 2 3 4 ... 9 Next", on mobile collapses labels into chevrons. | ||
| */ | ||
| export const Default: StoryComponent = () => <Pagination totalPages={9} defaultPage={3} />; | ||
| Default.storyName = 'Default'; | ||
|
|
||
| /** | ||
| * First page: the Previous control must be hidden (showPrevious logic). | ||
| */ | ||
| export const FirstPage: StoryComponent = () => <Pagination totalPages={10} defaultPage={1} />; | ||
| FirstPage.storyName = 'FirstPage'; | ||
|
|
||
| /** | ||
| * Last page: the Next control must be hidden. | ||
| */ | ||
| export const LastPage: StoryComponent = () => <Pagination totalPages={10} defaultPage={10} />; | ||
| LastPage.storyName = 'LastPage'; | ||
|
|
||
| /** | ||
| * Wide page count with active page in the middle: ellipses should appear on | ||
| * both sides of the visible range ("1 ... 9 10 11 ... 20"). | ||
| */ | ||
| export const WithEllipsis: StoryComponent = () => <Pagination totalPages={20} defaultPage={10} />; | ||
| WithEllipsis.storyName = 'WithEllipsis'; | ||
|
|
||
| /** | ||
| * No page list, only Previous/Next. On desktop the labels are visible, | ||
| * on mobile the labels are hidden and only chevrons are shown. | ||
| */ | ||
| export const NavOnlyResponsive: StoryComponent = () => ( | ||
| <Pagination totalPages={10} defaultPage={5} hidePageList /> | ||
| ); | ||
| NavOnlyResponsive.storyName = 'NavOnlyResponsive'; | ||
|
|
||
| /** | ||
| * Page list without navigation controls. Matches the product-list scenario | ||
| * from the Figma examples panel. | ||
| */ | ||
| export const PagesOnly: StoryComponent = () => ( | ||
| <Pagination totalPages={5} defaultPage={3} hideNavigationControls /> | ||
| ); | ||
| PagesOnly.storyName = 'PagesOnly'; | ||
|
|
||
| /** | ||
| * iconOnly mode forces chevron-only Previous/Next even on desktop. | ||
| * Combined with hidePageList matches the "Mapa / Listado" card scenario. | ||
| */ | ||
| export const IconOnlyControls: StoryComponent = () => ( | ||
| <Pagination totalPages={10} defaultPage={5} mode="iconOnly" hidePageList /> | ||
| ); | ||
| IconOnlyControls.storyName = 'IconOnlyControls'; | ||
|
|
||
| /** | ||
| * Compact view: triggered automatically when the viewport is narrower than | ||
| * 375px (high-zoom or space-limited contexts). Layout stacks vertically with | ||
| * Previous / page list / Next and only renders the current ± 1 pages. | ||
| * Only visible when the screenshot test runs this story at MOBILE_IOS_SMALL. | ||
| */ | ||
| export const CompactView: StoryComponent = () => <Pagination totalPages={50} defaultPage={24} />; | ||
| CompactView.storyName = 'CompactView'; | ||
|
|
||
| /** | ||
| * Pagination acting as a single next-chapter link: page list is hidden, only | ||
| * the Next control is rendered with a custom navRightLabel ("Siguiente | ||
| * capítulo"). Matches the chapter-reader scenario from the Figma examples. | ||
| */ | ||
| export const NextChapterLink: StoryComponent = () => ( | ||
| <Pagination totalPages={10} defaultPage={1} hidePageList navRightLabel="Siguiente capítulo" /> | ||
| ); | ||
| NextChapterLink.storyName = 'NextChapterLink'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import * as React from 'react'; | ||
| import {render, screen} from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import ThemeContextProvider from '../theme-context-provider'; | ||
| import {makeTheme} from './test-utils'; | ||
| import Pagination, {getPaginationItems} from '../pagination'; | ||
|
|
||
| test('renders pagination navigation landmark', () => { | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <Pagination totalPages={5} defaultPage={1} /> | ||
| </ThemeContextProvider> | ||
| ); | ||
|
|
||
| expect(screen.getByRole('navigation', {name: 'Paginación - Página 1 de 5'})).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('does not render when there is a single page', () => { | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <Pagination totalPages={1} /> | ||
| </ThemeContextProvider> | ||
| ); | ||
|
|
||
| expect(screen.queryByRole('navigation')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('calls onChange when a page button is clicked (uncontrolled)', async () => { | ||
| const onChange = jest.fn(); | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <Pagination totalPages={5} defaultPage={1} onChange={onChange} /> | ||
| </ThemeContextProvider> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByRole('button', {name: 'Ir a la página 3'})); | ||
|
|
||
| expect(onChange).toHaveBeenCalledWith(3); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see we always use the same number for testing things? 5 pages, call next expected 3.
|
||
| }); | ||
|
|
||
| test('calls onChange when Next is clicked', async () => { | ||
| const onChange = jest.fn(); | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <Pagination totalPages={5} defaultPage={2} onChange={onChange} /> | ||
| </ThemeContextProvider> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByRole('button', {name: 'Página siguiente'})); | ||
|
|
||
| expect(onChange).toHaveBeenCalledWith(3); | ||
| }); | ||
|
|
||
| test('does not change page when disabled', async () => { | ||
| const onChange = jest.fn(); | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <Pagination totalPages={5} defaultPage={1} onChange={onChange} disabled /> | ||
| </ThemeContextProvider> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByRole('button', {name: 'Ir a la página 3'})); | ||
|
|
||
| expect(onChange).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('honors controlled currentPage', () => { | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <Pagination totalPages={5} currentPage={3} /> | ||
| </ThemeContextProvider> | ||
| ); | ||
|
|
||
| // The current page is rendered as an aria-disabled button so VoiceOver can | ||
| // still land on it and announce "Página 3, página actual" — but the | ||
| // "Ir a la página 3" navigation button is absent (it's not navigable). | ||
| expect(screen.queryByRole('button', {name: 'Ir a la página 3'})).not.toBeInTheDocument(); | ||
| expect(screen.getByRole('button', {name: 'Página 3, página actual'})).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', {name: 'Ir a la página 2'})).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| describe('getPaginationItems', () => { | ||
| test('returns an empty array for a single page', () => { | ||
| expect(getPaginationItems({totalPages: 1, currentPage: 1})).toEqual([]); | ||
| }); | ||
|
|
||
| test('returns all pages when total fits without ellipsis', () => { | ||
| const items = getPaginationItems({totalPages: 5, currentPage: 3}); | ||
| expect(items.filter((i) => i.type === 'ellipsis')).toHaveLength(0); | ||
| expect(items).toHaveLength(5); | ||
| }); | ||
|
|
||
| test('inserts ellipsis when middle pages are skipped', () => { | ||
| const items = getPaginationItems({totalPages: 20, currentPage: 10}); | ||
| expect(items.some((i) => i.type === 'ellipsis')).toBe(true); | ||
| expect(items[0]).toMatchObject({type: 'page', page: 1}); | ||
| expect(items[items.length - 1]).toMatchObject({type: 'page', page: 20}); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks that previewTools is not working with Cyber skin, is this expected?
https://mistica-pl9771ukj-flows-projects-65bb050e.vercel.app/playroom#?code=N4Igxg9gJgpiBcIA8AFATjAbgSxgdwBUIIAbAZwD4kB6dLXQ48igHQDtUBDAc2zc4Au2CGwAE7UaP6YAMjABmAmZwBGMEgF4WIATDIDtEqZ0wAlbNwAWS1eq069BkEYEQBnEih56NwAIwADAC%2BRgC20DD2sPKcAK4kTuJiomCxaBhsAl7ckcA5AgDK7roAFNoADjx8gsJsAJKQbNkw2gA0ogDMAJQhySIAwpacbDm%2BJZU5XaIaFKJkMIXFMGUgE9VCIg0izW2iEzA97NQUIK06ljCheggA2iD9EKGhsWzYAgCeAPr972poIABdM54bBQASWMi3DoANgCAKCQA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm idk it does work for me...
Grabacion.de.pantalla.2026-06-09.a.las.16.05.01.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is the PreviewTools component that is broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Marcosld correct, we figured it out yesterday in pair programming. the skin is missing/not supported yet. I'd open a ticket to address it in a different branch. it's a quick and easy fix :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1569
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexandraGallipoliRodrigues we can resolve this I guess!