Skip to content
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.

Sign up for GitHub

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

Jump to bottom

コメントが多い場合の表示方法を変更 #5356

Merged
merged 7 commits into from
Aug 17, 2022
Merged

コメントが多い場合の表示方法を変更 #5356

merged 7 commits into from
Aug 17, 2022

Conversation

kei-kmj
Copy link
Contributor

@kei-kmj kei-kmj commented Aug 10, 2022 •
edited
Loading

Issue

概要

コメントが多い場合、表示を8件ずつ追加する

変更確認方法

  1. ブランチfeature/change_way_of_comments_loadingをローカルに取り込む
  2. bin/rails sでローカル環境を立ち上げる
  3. http://localhost:3000/products/1033498648 に管理者でアクセスする
  4. 以下を確認する
  • コメントが8件ずつ（又は最後の8件以内）追加される
  • 次のコメント()ボタンについて、コメントの残数が8超が、8以下かによって表示が変わる

@kei-kmj kei-kmj force-pushed the feature/change_way_of_comments_loading branch from 65fec73 to 659f726 Compare August 10, 2022 03:47
kei-kmj
kei-kmj commented Aug 10, 2022
View reviewed changes
app/javascript/comments.vue
@@ -150,6 +152,22 @@ export default {
changeActiveTab(tab) {
this.tab = tab
},
calcCommentIncrement: function () {
this.loadedComment =
Copy link
Contributor Author

@kei-kmj kei-kmj Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

分かり難いが、this.loadedCommentはBooleanを返す。
変数名を変更したかったが、既存の変数であり、今回のissueとは無関係の場所に影響があるので、変更していない。
変更するとしたらhasLoadedCommentAllとか、isLoadedCommentCompleteか

@kei-kmj kei-kmj marked this pull request as ready for review August 10, 2022 04:24
@kei-kmj kei-kmj requested a review from Nabegon August 10, 2022 04:24
@kei-kmj kei-kmj self-assigned this Aug 10, 2022
@kei-kmj
Copy link
Contributor Author

kei-kmj commented Aug 10, 2022

@Nabegon
レビューお願いできますか？急いでないです～

@Nabegon
Copy link
Contributor

Nabegon commented Aug 10, 2022

@kei-kmj お疲れ様です。すみません、今週は忙しいためこちらのレビューに対応するのが難しいです。他の方にご依頼いただけますと幸いです🙏

@kei-kmj kei-kmj requested review from yocaji and removed request for Nabegon August 10, 2022 07:58
@kei-kmj
Copy link
Contributor Author

kei-kmj commented Aug 10, 2022

@Nabegon
承知しました。連絡ありがとうございます～

@yocajii
レビュー可能ですか？

yocaji
yocaji reviewed Aug 11, 2022
View reviewed changes
Copy link

@yocaji yocaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

実装おつかれさまです 🍵
動作確認はばっちりでした✨

コードについて、1か所だけ気になった部分があって質問させていただきましたので、お手数ですがご確認のほどお願いいたします🙇‍♂️

app/javascript/comments.vue Outdated
Comment on lines 205 to 206
this.calcCommentIncrement()
this.showNextCommentsAmount()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finallyはfetch～thenの処理の成功/失敗に関わらず実行されるので、表示済みコメント数などを更新するような処理がここにあるのはちょっと違和感がありました。

レアなケースだとは思いますが、仮にfetchに失敗してコメントのデータが取得できなかった時、実際に表示できたコメントの数と残コメント数の表示が合わなくなってしまうように思います。

この処理は then(json) の中にある方が良さそうだと思いましたが、いかがでしょうか。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yocajii レビューありがとうございます🙏

2つの関数を1つにして、then(json)の中に移動しました。確認お願いします

@kei-kmj kei-kmj force-pushed the feature/change_way_of_comments_loading branch 3 times, most recently from 047aa79 to e51b9a7 Compare August 12, 2022 09:34
yocaji
yocaji reviewed Aug 14, 2022
View reviewed changes
Copy link

@yocaji yocaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

動作とコードの変更箇所を確認できました 🙆
1点、追加でコメントさせていただいたので、お手数ですがご確認のほどお願いいたします🙇

app/javascript/comments.vue Outdated
@@ -150,6 +152,20 @@ export default {
changeActiveTab(tab) {
this.tab = tab
},
displayMoreComments: function () {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すみません、最初に気付くことができていればよかったのですが、他のメソッドの書き方が省略記法になっているのでここも合わせておくと良さそうです。

Suggested change
displayMoreComments: function () {
displayMoreComments() {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yocajii
ありがとうございます！修正しました。確認お願いします🙏

@kei-kmj kei-kmj force-pushed the feature/change_way_of_comments_loading branch from e51b9a7 to 660a78c Compare August 15, 2022 00:02
yocaji
yocaji approved these changes Aug 15, 2022
View reviewed changes
Copy link

@yocaji yocaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ご対応ありがとうございます！
動作とコードを確認できましたので、Approveいたします 🙆

@kei-kmj kei-kmj requested a review from komagata August 17, 2022 05:28
@kei-kmj
Copy link
Contributor Author

kei-kmj commented Aug 17, 2022

@yocajii
ありがとうございました！

@komagata
レビューお願いします🙏

komagata
komagata approved these changes Aug 17, 2022
View reviewed changes
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit a95bfa2 into main Aug 17, 2022
@komagata komagata deleted the feature/change_way_of_comments_loading branch August 17, 2022 06:51
@github-actions github-actions bot mentioned this pull request Aug 17, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yocaji yocaji yocaji approved these changes

@komagata komagata komagata approved these changes

Assignees

@kei-kmj kei-kmj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kei-kmj @Nabegon @komagata @yocaji