Recursion-Group-K / sketch

https://sketch-frontend-d11d98fe289b.herokuapp.com/
0 stars 0 forks source link

refactor/drawing/vuex/color-weight #78

Closed sokahiri closed 2 years ago

sokahiri commented 2 years ago

本プルリクの目的

vuex周りのコードの書き方の共有、確認が主な目的です。

やったこと

確認してほしい事

コードレビューをお願いします。特にvuex関連のコード(drawing.js, vueコンポーネント内でのvuexの利用)の部分で不適切な書き方をしていないか確認を頂きたいです。

質問

redoなどのemitやrefを用いて実装している操作は全てvuexへ移行し、管理していくという認識で大丈夫でしょうか?また、それに伴って、vuexへ移行するメソッドに関わる変数(ポインター座標など)も全てvuexへ移行するという方針で実装していって良いでしょうか?

本プルリクで頂いたレビューを参考に、今後vuexへの移行を進めていきたいと思います!よろしくお願いいたします。

mcnLeandro commented 2 years ago

redoなどのemitやrefを用いて実装している操作は全てvuexへ移行し、管理していくという認識で大丈夫でしょうか?また、それに伴って、vuexへ移行するメソッドに関わる変数(ポインター座標など)も全てvuexへ移行するという方針で実装していって良いでしょうか?

はい!基本的にはそちらの認識で問題ないです!ちなみに、pointerはCanvas内でしか使っていないので、vuexに移行する必要はないかもしれないです!

key& keyup keydown ... redo&undo ..... localstorage....

のあたりが必要かなと思っていました! その他わからない部分があれば行ってください!

sokahiri commented 2 years ago

いつもありがとうございます!ご指摘いただいた点は修正させていただきますね! pointerの座標はキャンバス内でしか使ってないんですが、redoやundoメソッド内で使用しているので、pointerも一緒にstoreで管理する必要があるんじゃないかと思っているのですが、これを避ける方法はありますか?

mcnLeandro commented 2 years ago

@sokahiri そうですね。場合によっては確かにstoreに移動したほうがいいかもしれないかもですが、 今のところactionに引数を渡す形で実装していただけますか!

sokahiri commented 2 years ago

確かにその通りでした!ありがとうございます!

sokahiri commented 2 years ago

遅くなって申し訳ありません。指摘いただいた点の修正行いました!確認お願いします

sokahiri commented 2 years ago

ごめんなさい、忘れていました! lintしたものをpushしたのでお願いします!