sadがついた日報にコメントを書いて『確認OKにする』ボタンを押した際に『今日の気分は「sad」ですが、コメント無しで確認しますか？』が表示されないようにした #4239
Conversation
|
@Saki-htr こちらのレビューお願いいたします！ |
Aseiide
changed the title
Aseiide
changed the title
|
@Aseiide さん |
There was a problem hiding this comment.
@Aseiide
お疲れさまです。このたびはレビューのご依頼いただきありがとうございました。
私も同じように環境を作って、私だったらどう修正するだろうかと考えながらコードを拝見しました。
コメント投稿と確認チェックが同時に行われる際のコメントが既にある/ないの確認のタイミングを思ったとおりに動かすのが難しいですね。そしてcomments.vueにもcheckメソッドを作っておくというのはかなり工夫されたのだなという激闘ぶりが伝わってきました。
実装内容・挙動ともに不審な点はなく全く問題ないと思いましたので、私の方からはOKとさせていただきます👍
※テストについて1点確認していただきたいことがありましたので、別途コメントしました。
(確認・ご対応いただき次第、次の駒形さんのレビューに進んでください。)
よろしくお願いします😄
| wait_for_vuejs | ||
| click_button '確認OKにする' | ||
| end | ||
| wait_for_vuejs |
There was a problem hiding this comment.
このwait_for_vuejsの記述については、以前のテストからの流れで慣習的に残してある感じでしょうか。
先日、Maedaさんのプルリクで駒形さんが以下のリンクのようにコメントされているのを見ました。
#4141 (comment)
私のローカル環境でこのwait_for_vuejsの1行を削除してテストを行ってみましたが、特に問題なくテスト通過したようですので、ご確認いただき不要であれば削除、少々待つのが必要であれば上記のプルリクでMaedaさんが対応されていた方法が参考になるかなと思いました。
ご確認をお願いします。
There was a problem hiding this comment.
@Paru871
wait_for_vuejsを削除しても正常に動作しました。レビューありがとうございます！
Aseiide
force-pushed
the
bug/dont-show-sad-popup-window-with-mentor
branch
from
45ab188 to
d49c83d
Compare
|
@komagata |
| } | ||
| const check = document.getElementById('js-shortcut-check') | ||
| this.createComment() | ||
| check.click() |
There was a problem hiding this comment.
これってなぜこの方法から、自前でPOSTする方法に変更したんでしょう？
There was a problem hiding this comment.
「確認OKにする」ボタンには、「日報を確認」する機能が必要で、ポップアップは必要ない。
「日報を確認」ボタンには、「日報を確認」する機能と、ポップアップの両方必要。
「日報を確認」するというメソッド(機能)はcheck.vueに存在し、comments.vueからは呼べないと思いました。
なぜなら、親子関係にないためです。
なので、「日報を確認」する機能に該当する部分(変更後コードのcomments.vue L138~L164)をcheck.vueから移植して自前でPOSTするようにしました。
自分では思いつかなかったのですが、comments.vueからcheck.vueのcheckメソッドを呼ぶ方法がございましたら、実装のヒントや方針などご教示いただけますと幸いです。
There was a problem hiding this comment.
@Aseiide なるほどですね〜
Checkする処理は同じだと思うのでcomponentからmixinを使って共通化するのはいかがでしょうか〜
There was a problem hiding this comment.
@komagata
なるほど、そういう方法があるのですね。
mixin調べてやってみます。
| } | ||
| const check = document.getElementById('js-shortcut-check') | ||
| this.createComment() | ||
| check.click() |
There was a problem hiding this comment.
「確認OKにする」ボタンには、「日報を確認」する機能が必要で、ポップアップは必要ない。
「日報を確認」ボタンには、「日報を確認」する機能と、ポップアップの両方必要。
「日報を確認」するというメソッド(機能)はcheck.vueに存在し、comments.vueからは呼べないと思いました。
なぜなら、親子関係にないためです。
なので、「日報を確認」する機能に該当する部分(変更後コードのcomments.vue L138~L164)をcheck.vueから移植して自前でPOSTするようにしました。
自分では思いつかなかったのですが、comments.vueからcheck.vueのcheckメソッドを呼ぶ方法がございましたら、実装のヒントや方針などご教示いただけますと幸いです。
| } | ||
| const check = document.getElementById('js-shortcut-check') | ||
| this.createComment() | ||
| check.click() |
There was a problem hiding this comment.
@Aseiide なるほどですね〜
Checkする処理は同じだと思うのでcomponentからmixinを使って共通化するのはいかがでしょうか〜
|
@komagata |
Aseiide
force-pushed
the
bug/dont-show-sad-popup-window-with-mentor
branch
from
5bf9076 to
8df3684
Compare
issue
概要(再現手順)
sadの日報の時に、「確認OKにする」ボタンを押したときはポップアップが出ないように変更しました。
変更前
「日報を確認」ボタンを押したときも、「確認OKにするボタン」を押したときもポップアップが出てしまっている
PR.4239-.mov
変更後
「確認OKにするボタン」を押したときは、ポップアップが出ないようにした。
PR.4239-.mov