feat(snippets): confirm before discussing #721
Conversation
There was a problem hiding this comment.
Je suis pas fan d'avoir fusionné les controllers.
Pour la facilité de maintenabilité et de compréhension, la philosophie des controllers Stimulus est plutôt d'encapsuler un comportement par controller (ou quelques comportements étroitement liés). Ici on est en train de faire devenir ça un controller plus proche d'un composant à la React, qui pilote toutes les interactions d'un élément graphique spécifique. On sort du cadre prévu de l'outil et j'ai peur qu'on finisse par se battre contre Stimulus.
Pas atroce mais juste mon gut feeling à la lecture 🙂
There was a problem hiding this comment.
En l'occurence sans les mergé on se retrouverait avec 3 controller different utilisé sur le meme element html et qui ont fonctionnalité propre a ce seul element donc je trouvais que ca faisait plus de sens de tout mettre ensemble.
Je vais les reseparer du coup
wJoenn
force-pushed
the
feat/snippets/confirm-before-discussing
branch
5 times, most recently
from
35e70c0 to
a96e5a6
Compare
wJoenn
force-pushed
the
feat/snippets/confirm-before-discussing
branch
from
a96e5a6 to
8f8aa4c
Compare
|
J'ai pas testé en local, mais le code me paraît plus respectueux du Single Responsibility Principle 👍 @Aquaj si c'est bon pour toi c'est bon pour moi ! |
* refactor(snippets): js:ReactionController -> SnippetsController * feat(snippets): add handleDiscuss to js:SnippetsController * feat(snippets): require slack_linked? to discuss * refactor(snippets): move JS:ShowMoreController to JS:SnippetsController * fix: linter * refactor(js): externalise expandable logic from JS:snippetsController * refactore(js): externalise confirm ogic from js:SnippetsController * refactor(js): rename js:SnippetsController -> ReactionsController * refactor: undo useless changes Co-authored-by: Louis Ramos <75388869+wJoenn@users.noreply.github.com>
Summary of changes and context
Le goal de cette PR est d'afficher une alerte JavaScript avant de creer un thread slack pour eviter que les users des snippets concerné soient tag pour qu'ensuite personne ne demande rien.
Je suis partit du principe que cela arrivait a cause d'un manque de comprehension de ce que le bouton ferait.
Mon idée etait donc que, dans le cas ou le thread n'existe pas encore, un confirm dialog apparaitrait donnant plus de contexte au user qu'il devra ensuite confirmer pour que le thread soit creer dans Slack. Une fois confirmé une nouvelle page s'ouvre et cette page redirige vers Slack comme actuellement.
Si le thread existe deja alors aucune confirmation n'est requise et le user est directement redirigé vers slack
Ma premiere approche etait d'utiliser
turbo_confirmsur le bouton pour afficher le message mais j'ai decouvert qu'il n'etait pas possible d'associertarget: :_blanketdata-turbo-confirmsur un meme form du coup pour queturbo_confirmfonctionne il aurait fallut qu'on redirige depuis la meme page ce qui serait une regression par rapport a la UX actuelleLa deuxieme solution etait de creer un controller Stimulus pour gerer le confirm et le submit par nous meme. C'est ce que j'ai fais Ca aurait été le 3ème controller utilisé dans
box_componentet seulement a cet endroit donc j'ai decidé de refactoré leJS:ReactionsControlleret leJS:ShowMoreControlleren uyn uniqueJS:SnippetsControlleret d'ajouter une nouvelle methodehandleDiscussdedans.Sanity checks