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

[FEATURE] Ajouter la possibilité d'éxécuter une function lors de l'utilisation de la PixPagination (PIX-15863) #798

Conversation

xav-car
Copy link
Contributor

@xav-car xav-car commented Dec 24, 2024

🎄 Problème

PixPagination ne permet pas a l'heure actuel d'éxecuter une function lors du changement de page, ce qui s'avère utile dans certains context

🎁 Proposition

Ajouter la possibilté de mettre une function onChange qui sera appelée au suivant/précédent ou au changement de pageSize

🌟 Remarques

Changement du getter pageSize qui était forcément en erreur dans le cas ou l'on ne passait pas de pagination avec une valeur sur PageSize.

🎅 Pour tester

CI au vert, vérification que cette modification permet la fonctionnement actuel de la page

@pix-bot-github
Copy link

Une fois l'application déployée, elle sera accessible à cette adresse https://ui-pr798.review.pix.fr
Les variables d'environnement seront accessibles sur scalingo https://dashboard.scalingo.com/apps/osc-fr1/pix-ui-review-pr798/environment

@xav-car xav-car force-pushed the pix-15863/authorize-onChange-args-on-pix-pagination branch 2 times, most recently from 3073555 to a3a6a0c Compare December 26, 2024 08:35
Comment on lines +109 to +111
if (typeof this.args.onChange !== 'function') return;

this.args.onChange();
Copy link
Contributor

Choose a reason for hiding this comment

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

question: N'est-il pas préférable de laisser le navigateur péter une erreur tout seul si le onChange de la ligne this.args.onChange() n'est pas une fonction ?

Dans ce cas, je trouve que l'ajout de cette action n'est pas forcément nécessaire, on pourrait utiliser directement la prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

proposition: aussi, ce serait pas mal que l'action renvoie les infos de pagination (ne serait-ce que la nouvelle page en cours) ?
C'est plus efficace que de devoir retrouver à la main les infos dans les query params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comme le onChange n'est pas obligatoire.je n'ai pas envie que le navigateur pète si c'est autre chose qu'une function (j'aurai pu ajouter un ember/warn au constructor)

Pour le moment c'etait plus le fait d'être ISO avec un besoin qu'on a sur PixOrga. (Permettre d'exécuter une fonction au changement de la page, reset de tableau de suppression de ligne)

Le changement est intéressant. Mais qui nécessitera un breaking change, et un alignement avec les équipes si l'on ne doit plus passer par le router dans le PixPagination mais seulement par le onChange.

Copy link
Contributor

@alicegoarnisson alicegoarnisson left a comment

Choose a reason for hiding this comment

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

Tech OK, func OK 🦦

@pix-service-auto-merge pix-service-auto-merge force-pushed the pix-15863/authorize-onChange-args-on-pix-pagination branch from 5b94b89 to ab010f9 Compare January 8, 2025 10:34
@pix-service-auto-merge pix-service-auto-merge merged commit 8f746de into dev Jan 8, 2025
5 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the pix-15863/authorize-onChange-args-on-pix-pagination branch January 8, 2025 10:37
pix-service-auto-merge pushed a commit that referenced this pull request Jan 8, 2025
# [52.2.0](v52.1.0...v52.2.0) (2025-01-08)

### 🚀 Amélioration

- [#798](#798) Ajouter la possibilité d'éxécuter une function lors de l'utilisation de la PixPagination (PIX-15863)
@pix-service-auto-merge
Copy link
Contributor

🎉 This PR is included in version 52.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants