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

個人の日報一覧をプラクティスで絞り込めるようにした #6044

Merged
merged 16 commits into from Feb 12, 2023

Conversation

peno022
Copy link
Contributor

@peno022 peno022 commented Jan 13, 2023

Issue

概要

個人の日報一覧(https://bootcamp.fjord.jp/users/1463/reports など)を、プラクティスで絞り込めるようにしました。
(全ユーザーの日報一覧( https://bootcamp.fjord.jp/reports )には、既に同様の絞り込み機能が実装されています。)

変更確認方法

  1. feature/filter-user-reports-by-practiceをローカルに取り込む
  2. 任意のユーザーでログイン
  3. komagataさんの日報一覧( http://localhost:3000/users/459775584/reports )を表示
  4. セレクトボックスが表示されていること、プラクティスで日報を絞り込めることを確認します。テストデータ上は、OS X Mountain Lionをクリーンインストールするのプラクティスに、紐づく日報が存在しています。

Screenshot

変更前

スクリーンショット 2023-01-13 10 44 20

変更後

スクリーンショット 2023-01-13 10 40 24

スクリーンショット 2023-01-13 10 40 32

スクリーンショット 2023-01-13 10 40 55

@peno022 peno022 marked this pull request as ready for review January 13, 2023 04:35
@peno022
Copy link
Contributor Author

peno022 commented Jan 13, 2023

@Seakimhour
おつかれさまです!こちらのPRのレビューをお願いしてもよいでしょうか?

Copy link
Contributor

@Seakimhour Seakimhour left a comment

Choose a reason for hiding this comment

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

@peno022 お疲れ様です。
レビュー依頼ありがとうございます!
確認させていただきました、問題ないと思うのでApproveさせていただきました!

@peno022 peno022 requested a review from komagata January 13, 2023 08:13
@peno022
Copy link
Contributor Author

peno022 commented Jan 13, 2023

@Seakimhour
速やかに見ていただきありがとうございます!
@komagata
レビューよろしくお願いいたします!

@komagata
Copy link
Member

komagata commented Jan 17, 2023

@peno022 すみません、こちら画面遷移無しで(できればVue.jsよりReactを使って)絞り込めるようにお願いしたいです〜。

#6037 (comment)

@peno022 peno022 force-pushed the feature/filter-user-reports-by-practice branch from de0eae2 to 55c7cbc Compare January 28, 2023 08:34
@peno022 peno022 force-pushed the feature/filter-user-reports-by-practice branch 2 times, most recently from d4e6ba6 to e84ac15 Compare February 3, 2023 06:56
@peno022
Copy link
Contributor Author

peno022 commented Feb 3, 2023

@Seakimhour
おつかれさまです!
こちら以前一度Approveいただいたのですが、「画面遷移なしで」絞り込む、という要件を見落しておりまるっと再修正したため、再度レビューお願いしてもよいでしょうか?

Copy link
Contributor

@Seakimhour Seakimhour left a comment

Choose a reason for hiding this comment

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

@peno022 新たにレビューさせていただきます~

Reactの経験がないです、よろしくお願いいたします!:cat:

一つ点です、下に書きました。
他のはいいと思います!

</a>
</li>
</ul>
<label className="a-overlay" for={report.id}></label>
Copy link
Contributor

@Seakimhour Seakimhour Feb 3, 2023

Choose a reason for hiding this comment

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

Suggested change
<label className="a-overlay" for={report.id}></label>
<label className="a-overlay" htmlFor={report.id}></label>

ここもhtmlForに変更が必要ですねー

const ReportListItemActions = ({ report }) => {
return (
<div className="card-list-item-title__end">
<label className="card-list-item-actions__trigger" for={report.id}></label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<label className="card-list-item-actions__trigger" for={report.id}></label>
<label className="card-list-item-actions__trigger" htmlFor={report.id}></label>

ここはhtmlForに利用したらどうでしょうか?

@peno022
Copy link
Contributor Author

peno022 commented Feb 3, 2023

@Seakimhour
ありがとうございます〜!修正しましたので再度ご確認お願いできますでしょうか!

Copy link
Contributor

@Seakimhour Seakimhour left a comment

Choose a reason for hiding this comment

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

@peno022 確認をしました。問題ないと思いますので、Approveさせていただきます。

@peno022
Copy link
Contributor Author

peno022 commented Feb 4, 2023

@Seakimhour
ありがとうございましたー!

@komagata
キムさんレビューおわりましたので、駒形さんレビューお願いいたします!

</div>
</div>
{(report.hasAnyComments) && (
<ReportListComment report={report} />
Copy link
Member

Choose a reason for hiding this comment

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

Commentは他のページでも使うので別ファイルのコンポーネントにした方がいいかもです〜

@peno022
Copy link
Contributor Author

peno022 commented Feb 8, 2023

@machida ( @komagata )

おつかれさまです!
このPRで、個人の日報一覧(https://bootcamp.fjord.jp/users/[user_id]/products )をReact化し、かつプラクティスでの絞り込みを入れる対応をしています。

一覧のコメント表示部分を共通化のためにコンポーネントとして切り出したいと思っているのですが、仕様について質問させてください。
「コメント表示部分」は、以下画像の赤枠の部分を指しています。
スクリーンショット 2023-02-08 19 31 44

個人の日報一覧と提出物一覧で、コメント表示部分に区切り線の有無で違いがあったので、
「そもそもこの違いは意図されているものなのか?」
をご確認したいです。
(区切り線があるほうが見やすそう?)

下記のどちらにするかを決めたいと思っています。

  • 意図されているものであれば、区切り線とそれ以外を別コンポーネントで作る
  • 意図されていない(ので今後揃えていく)のであれば、揃えるほうの状態にあわせて1つのコンポーネントで作る

表示の違い

①日報一覧は、区切りの点線が入る

スクリーンショット 2023-02-08 19 31 35

②提出物一覧は、区切りの点線が入らない。

スクリーンショット 2023-02-08 19 31 53

@machida
Copy link
Member

machida commented Feb 8, 2023

@peno022 区切り線のある方でお願いしますー
提出物の方は何かミスがあって線が出てなさそうです。こちらは別Issueで修正を進めますー💪

@peno022 peno022 force-pushed the feature/filter-user-reports-by-practice branch from 7a58664 to f75c787 Compare February 9, 2023 01:21
@peno022
Copy link
Contributor Author

peno022 commented Feb 9, 2023

@komagata
修正しました!再度レビューお願いいたします。🙏

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です〜🙆‍♂️
難しいIssueをありがとうございます〜!

@komagata komagata merged commit e4afa7e into main Feb 12, 2023
@komagata komagata deleted the feature/filter-user-reports-by-practice branch February 12, 2023 13:56
@github-actions github-actions bot mentioned this pull request Feb 12, 2023
14 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