Feat/asso/membership #31
Conversation
|
|
||
| const api = useAPI(); | ||
|
|
||
| const updateItems = (query: Record<string, string>) => { |
There was a problem hiding this comment.
Passer page en param ? Et tu l'ajoutes auto dans query ?
There was a problem hiding this comment.
Cad, tu parles de séparer page ici en une propriété distincte ? En soit elle est pas censée être plus utilisée que ça ici puisque c'est lors de la mise à jour du filtre, ce serait bizarre de commencer en skippant la page 1. Après c'est quand même possible de préciser la page dans la query mais pas forcément ce qu'on veut je trouve
There was a problem hiding this comment.
Yes c'est vrai.
Du coup, si page n'est jamais passé, autant ne juste pas considérer qu'il puisse être passé (peut-être même l'interdire ? Comme ça on s'assure qu'il n'y a pas de trucs chelous si jamais).
S'il peut-être passé, je trouve ça bizarre de le passer dans la query, il a une valeur différente que tout autre élément de query dans la fonction.
There was a problem hiding this comment.
Je dirais que c'est au développeur de ne pas faire n'importe quoi personnellement. Je considère qu'éventuellement dans certains cas on pourrait vouloir commencer à la page y (ex. si une url inclut un numéro de page dans une pagination quelconque)
There was a problem hiding this comment.
Et donc pourquoi pas ça ?
S'il peut-être passé, je trouve ça bizarre de le passer dans la query, il a une valeur différente que tout autre élément de query dans la fonction.
Ce que je veux dire par là, c'est pourquoi pas un autre param que query, pour passer la page, puisque c'est pas un élément classique
experimenting ch conversion to em, with a (wrong) basis of 1ch = 0.5em for style clarity
AlbanSdl
force-pushed
the
feat/asso/membership
branch
from
eaa757a to
fa3ebae
Compare
* does not build yet, still requires the typescript upgrading coming in PR33
|
Petite précision : je dis que j'ai fix le build, c'est partiellement le cas. La raison pour laquelle ça ne build pas encore est que notre version de typescript n'est pas assez forte pour inférer deux types, ça devrait être réglé par la pr 33. |
| } | ||
| } | ||
|
|
||
| &.size-small { |
There was a problem hiding this comment.
size_small, idem pour les autres (sauf s'il y a une raison particulière)
There was a problem hiding this comment.
Bah en soit, c'est absolument pas idiomatique du css les underscores... Tu mets des tirets dans tes noms de classes normalement
There was a problem hiding this comment.
Yes, mais nous on utilise les _ pour que ça soit utilisable comme nom de variable. Enfin on peut changer ça si tu préfères, fondamentalement pourquoi pas, mais dans une autre PR
| </Button> | ||
| <Button onClick={() => deleteAssoMember(member.id)} disabled={!hasEditMembersPermission}> | ||
| </DisableableButton> | ||
| <DisableableButton |
There was a problem hiding this comment.
J'y pense : pourquoi tous les boutons sont pas des disableable buttons ?
| }) | ||
| .on('fallback', () => delete handler.current) | ||
| .on(ResponseFailureReason.timeout, () => {}); // Hide toast error here as we might have aborted the request | ||
| .on(ResponseFailureReason.abort, () => {}); // Hide toast error here as we might have aborted the request |
There was a problem hiding this comment.
tu peux enlever la fin du comment, on a forcément abort la req du coup
No description provided.