-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Proposition d'alternative au DereferenceMixin - Gestion de MembershipAssociation côté frontend #175
Conversation
ae3ba67
to
dfa418b
Compare
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.
Merci !
Cela fait un long moment qu'on se disait que ce serait plus cohérent que le code soit géré au niveau du frontend (cf. assemblee-virtuelle/semapps#891 et assemblee-virtuelle/semapps#894).
Je n'ai pas testé le code, mais il me semble très lisible, ce qui n'a pas été le cas jusqu'à maintenant.
Dans le futur, l'idéal serait d'avoir un composant plus générique, qui puisse s'adapter à tous les types de relations réifiées. Mais ça impliquerait sans doute d'ajouter un grand nombre de props... A mon avis, si on arrive déjà à avoir quelque chose de clair et fonctionnel sur Archipelago, c'est déjà une super avancée. On pourra voir plus tard pour faire du générique.
Je laisse @simonLouvet confirmer si ça correspond à tous les besoins côté Archipelago.
On pourra effectivement virer les composants du package @semapps/semantic-data-provider, qui n'étaient de toute façon pas documentés et qui n'étaient pas à leur place dans ce package.
frontend/src/resources/Agent/Actor/Organization/OrganizationEdit.js
Outdated
Show resolved
Hide resolved
C'est ok pour moi après une petite h de revue. Un grand merci @mguihal pour faire avancer ce chantier. Les composants étaient génériques et deviennent spécifiques mais ils ne servent que dans archipelago dans le cas des relations réifiées donc on peut rester comme ca pour le moment. Un petit commentaire pour faire du 100% spécifique pour éviter les confusions d'avoir des composants paramétrables qui ne le sont pas. Je rappelle que la migration des relations réifiées vers le front n'est pas terminée puisque c'est le mixin Je pense qu'il faut garder les mixins Je me suis permis de merge #174 suite à la prise en compte de la review de @srosset81 : Ne pas oublier de supprimer le mixin sur les containers users et oganizations. |
@simonLouvet Je suis OK pour intégrer le |
5a48a31
to
61bffe1
Compare
Hello, |
61bffe1
to
c61fa6d
Compare
Hello,
Suite aux récents déboires avec la gestion des relations réifiées (Organization <-> MembershipAssociation <-> Person), et du
dereferenceMixin
qui avait été rajouté dans Archipelago mais qui un coup fonctionne (#133, #175), un coup ne fonctionne plus (#5, #45, #173), un coup est commenté dans le code (#158), je me suis mis en peine de réfléchir à une solution alternative, plus dans l'esprit de react-admin, en gérant le fetch des données de la relation côté front, au lieu de le faire côté middleware.J'aimerais bien avoir votre avis éclairé et constructif @srosset81 et @simonLouvet sur cette proposition, pour savoir si selon vous cette solution répond à tous les usages pour lesquels nous avions besoin de déférencement d'une part. Et pour savoir s'il est préférable de faire cette logique côté front (comme proposé ici), ou côté middleware (comme c'est fait actuellement).
PS : @simonLouvet J'ai commencé à réfléchir et à implémenter cette proposition avant que tu ne proposes la correction sur le derefenceMixin hier, ne sachant pas si le bug détecté pouvait être résolu facilement et rapidement 😬
PS2 : Je n'ai fait pour l'instant que la modification côté frontend, mais si cette solution venait à être validée, il faudra faire les modifications nécessaires côté middleware aussi (ainsi que sur Semapps pour supprimer les composants liés dans le semantic-data-provider éventuellement)