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.

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

日報個別ページにその人の直近数個の日報を表示する(Vue.jsで) #3118

Merged
merged 21 commits into from
Dec 3, 2021

Conversation

harukus
Copy link
Contributor

@harukus harukus commented Aug 11, 2021

issue

#2649
(#2660 の作り直し)
#3130

やったこと

  • 提出物を確認した時に右側に直近の日報が表示されているのと同じように、日報にもその人の直近の日報が表示されるようにした。(すべてのユーザーが見ることができる。)

提出物での表示

スクリーンショット 2021-09-04 14 42 11

日報での表示

スクリーンショット 2021-09-11 10 39 54

@harukus
Copy link
Contributor Author

harukus commented Sep 4, 2021

@machida
日報のデザインの修正お願いいたしますm(_ _)m

@machida
Copy link
Member

machida commented Sep 6, 2021

了解ですー

@machida machida self-assigned this Sep 6, 2021
@machida
Copy link
Member

machida commented Sep 8, 2021

@harukus 2カラムにしましたー

@machida machida removed their assignment Sep 8, 2021
@harukus
Copy link
Contributor Author

harukus commented Sep 10, 2021

@machida ありがとうございます!

@harukus harukus marked this pull request as ready for review September 11, 2021 01:26
@harukus
Copy link
Contributor Author

harukus commented Sep 11, 2021

@sinsoku
遅くなってごめんなさいm(_ _)m
#2660 でのレビューについて修正いたしました。お時間ある時に確認いただけると幸いです🙇‍♂️

Comment on lines 46 to 55
.then((response) => {
return response.json()
})
.then((json) => {
this.reports = []
json.reports.forEach((r) => {
this.reports.push(r)
})
this.currentUserId = json.currentUserId
})
Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScriptはあまり詳しくないので、コードを読んで気になった点を質問させてください!

  • なぜ then を2回使っているのか
  • forEach, push を使っている理由
Suggested change
.then((response) => {
return response.json()
})
.then((json) => {
this.reports = []
json.reports.forEach((r) => {
this.reports.push(r)
})
this.currentUserId = json.currentUserId
})
.then((response) => {
const json = response.json()
this.reports = json.reports
this.currentUserId = json.currentUserId
})

then を1つにして、 reports もそのまま代入できそうかなと思ったので。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sinsoku
返信が遅くなって申し訳ありません。

  • なぜ then を2回使っているのか
    説明が合っているかわからないのですが、fetchメソッドはPromiseを返し、PromiseはResponseオブジェクトとして解決されるためです。
  • forEach, push を使っている理由
    こちらは参考にしたPRのコードをそのまま真似したため、簡潔に書きなおしました。

確認お願いいたします。

Copy link
Contributor

Choose a reason for hiding this comment

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

@harukus
thenは1つにしても動きそうかなと思ったのですが、これだと動かなくなったりするでしょうか?

.then((response) => {
  const json = response.json()
  this.reports = json.reports
  this.currentUserId = json.currentUserId
})

コードが短くなるので、動くなら短いコードの方が良いなと思いました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sinsoku
上記のコードでは直近の日報が表示されなくなってしまいました><
スクリーンショット 2021-11-05 10 10 56

Copy link
Contributor

Choose a reason for hiding this comment

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

あれ、動かないのか。
それなら今のコードのままで良さそうです!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sinsoku
ありがとうございます!

@komagata komagata assigned komagata and machida and unassigned harukus Nov 9, 2021
@komagata komagata force-pushed the feature/display-recent-reports branch from 1da046c to e63e510 Compare November 13, 2021 07:56
@komagata
Copy link
Member

@machida ちょっとこのブランチの日報個別ページのslimが最新と差分が大き過ぎてコンフリクト修正時に間違ってしまったかもです。デザインがずれてしまったのですがちょっと確認してもらってよいでしょうか、すみません。

スクリーンショット 2021-11-13 16 57 13

@machida machida force-pushed the feature/display-recent-reports branch from e63e510 to 277efef Compare November 24, 2021 16:17
@machida
Copy link
Member

machida commented Nov 24, 2021

@komagata デザイン調整を行いましたー 見た目はこれで大丈夫なのですが、routes のコンフリクトを解消したのでコードの方のご確認お願いします🙏

直近の日報は全ユーザーが見れるようにしました。

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
Copy link
Member

@machida 修正ありがとうございます!
テストが落ちているので再実行しておきました。

@machida machida force-pushed the feature/display-recent-reports branch from 277efef to e2d7a1d Compare November 30, 2021 04:52
@machida
Copy link
Member

machida commented Nov 30, 2021

@komagata テスト通しましたー

@machida machida removed their assignment Nov 30, 2021
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 8913025 into main Dec 3, 2021
@komagata komagata deleted the feature/display-recent-reports branch December 3, 2021 10:18
@github-actions github-actions bot mentioned this pull request Dec 3, 2021
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants