Skip to content
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

feat(Pagination): Avoir une valeur props pour la page courante. #206

Merged
merged 10 commits into from
Apr 12, 2021

Conversation

hdeschenes-equisoft
Copy link
Contributor

Cette PR ajoute la props "activePage" sur la pagination pour avoir plus de contrôle sur le state de celle-ci. J'ai ajouté un example dans le Storybook qui s'appelle LinkedPagination avec deux paginations qui partage le même state "activePage".

https://jira.equisoft.com/secure/RapidBoard.jspa?rapidView=786&projectKey=DS&view=detail&selectedIssue=DS-173&quickFilter=4537

@hdeschenes-equisoft hdeschenes-equisoft requested review from a team and maxime-gendron February 9, 2021 19:04
@hdeschenes-equisoft hdeschenes-equisoft changed the title Dev/ds 173 feat(Pagination): Avoir une valeur props pour la page courante. Feb 11, 2021
onPageChange={(page: number) => setCurrentPage(page)}
activePage={currentPage}
/>
<Pagination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pourquoi la pagination est présente deux fois?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est un use case dans les maquettes actuelle sur le projet Centralize, on veux avoir 2 paginations synchro en haut et en bas d'une Table.

Copy link
Contributor Author

@hdeschenes-equisoft hdeschenes-equisoft Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bref j'ai enlevé le deuxième et changer le nom pour ControlledInput

@@ -16,6 +16,27 @@ export const Normal: Story = () => (
<Pagination totalPages={50} numberOfResults={1530} />
</>
);

export const LinkedPagination: Story = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je mettrais ControlledPagination, pour faire référence aux controlled inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça fais du sens je ferais la modification

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est fait !

setCurrentPage(activePage);
}
}, [activePage]);

function changePage(page: number): void {
setCurrentPage(page);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sinon, peut-être juste appeler setCurrentPage ici quand activePage n'est pas défini

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est fait !

packages/react/src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved
const [currentPage, setCurrentPage] = useState(1);

return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pas besoin du fragment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est fait

@@ -116,6 +121,7 @@ export function Pagination({
defaultActivePage = 1,
pagesShown = 3,
onPageChange = () => undefined,
activePage,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Petit dernier détail, il faudrait changer la ligne 129 pour changer
const [currentPage, setCurrentPage] = useState(clamp(defaultActivePage, 1, totalPages));
pour
const [currentPage, setCurrentPage] = useState(clamp(activePage || defaultActivePage, 1, totalPages));
pour prendre en compte le activePage au premier render.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est fait

@meriouma meriouma merged commit f847e26 into master Apr 12, 2021
@meriouma meriouma deleted the dev/DS-173 branch April 12, 2021 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants