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

コースごとの書籍一覧ページを作成 #7152

Merged
merged 5 commits into from
Dec 31, 2023

Conversation

2525nicole
Copy link
Contributor

@2525nicole 2525nicole commented Dec 17, 2023

Issue

概要

コースごとに必要な参考書籍一覧ページを新たに作成いたしました。

変更確認方法

  1. feature/create-book-list-for-each-courseをローカルに取り込む
  2. rails db:seedで本PRで追加した書籍のデータを development 環境に読み込む
  3. foreman start -f Procfile.devを実行し、ローカル環境を立ち上げる
  4. komagataでログインし、Railsプログラマーコースページにアクセスする
  5. 「書籍」タブをクリックする

スクリーンショット 2023-12-17 11 35 27

  1. Railsプログラマーコースのプラクティス「OS X Mountain Lionをクリーンインストールする」の参考書籍が表示されていることを確認する
  • 「全て」表示の時は必読書と必読ではない本どちらも表示される
スクリーンショット 2023-12-21 18 52 56
  • 「必読」表示の時は必読書だけが表示される
スクリーンショット 2023-12-21 18 57 02

Screenshot

変更前

  • 各コースページではそのコースのプラクティスのみが表示されている
    image

  • 参考書籍ページではコースごとに必要な書籍に絞り込むことはできない
    スクリーンショット 2023-12-17 11 58 27

変更後

  • 各コースページの「書籍」タブにそのコースの参考書籍一覧が表示される
    • 必読書だけの一覧表示も可能

Image from Gyazo

@2525nicole
Copy link
Contributor Author

@natsuto6
おつかれさまです!
既にレビュー依頼をお持ちのところ申し訳ないのですが、
ポイントの状況等を鑑みて依頼のご相談をさせていただきました😣
レビューをお願いすることは可能でしょうか🙏
全く急ぎではありませんので、ご都合のよろしいタイミングで見ていただくかたちで問題ありません🙇‍♀️

@2525nicole 2525nicole marked this pull request as ready for review December 17, 2023 15:30
@2525nicole 2525nicole changed the title Feature/create book list for each course コースごとの書籍一覧ページを作成 Dec 17, 2023
@Takuya-Sakai91
Copy link
Contributor

@2525nicole
承知しました👌今週立て込んでおりますがなる早で対応させていただければと思います🙇‍♂️
取り急ぎ気になったのですが、私を含め「Vueを消し去るのが目的」のIssueに取り組んでいますが、Vueで書かれているのは何か理由があってのことなのでしょうか?komagataさんからも以前ご連絡がありましたが、Reactのようなフレームワークを使わないただのJSで作っても大丈夫とのことですが、Vueで書かれている理由を知りたかった次第です。

@2525nicole
Copy link
Contributor Author

@natsuto6
お引き受けいただきありがとうございます😭!
natsuto6さんの落ち着かれたタイミングで全く問題ございません🙇‍♀️
お忙しいところ恐れ入りますが、よろしくお願いいたします🙏

Vueで書いた理由について

こちらのIsuueを割り振っていただいた際の開発MTGで以下のようにいただきました。

  • Vue、JS、Reactどれで書いてもよいが、Vueで書いて別IssueでReact化をするのが良いかもしれない
  • 万が一JSやReactで書く場合はポイントを増やすことも可

これを受けて、以下のようなIssueの分け方がしっくりきたため今回はVueで書くことを選択しました🙇‍♀️

  • 今回はVueで書かれている/booksページをを使用して必要なページ(コースごとの書籍一覧ページ)を作る
  • 別IssueでReact化

ご確認の程よろしくお願いいたします!

Copy link
Contributor

@Takuya-Sakai91 Takuya-Sakai91 left a comment

Choose a reason for hiding this comment

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

@2525nicole
気になったところがいくつかございましたのでご確認をお願いします🙇‍♂️

  • リロード時や、「全て」タブから「必読」タブに切り替えた際などに、「プラクティスで絞り込む」のプルダウンが常に全プラクティスの参考書籍を表示に戻ってしまいますが想定通りでしょうか?
    そうでない場合、ユーザーが選択したプラクティスがリロードやタブ切り替え後も保持される方が良いと思いました。
    Image from Gyazo

  • 3つの書籍しか登録されていませんが、UIの確認やテストを行うにはもう少し書籍データがあっても良いのかなと思いましたがいかがでしょうか🙏

.o-empty-message__icon
i.fa-regular.fa-sad-tear
p.o-empty-message__text
| 登録されている本はありません
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

@2525nicole 2525nicole Dec 20, 2023

Choose a reason for hiding this comment

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

ご確認ありがとうございます🙇‍♀️
こちらですがアプリ内に参考書籍自体の登録が1冊も存在しない場合に表示されるもののようです👀

.o-empty-message(v-else)
.o-empty-message__icon
i.fa-regular.fa-sad-tear
p.o-empty-message__text
| 登録されている本はありません

✏️以下、v-elsegetBooks()から取得したbooksのlengthが0の時、というのが条件のため

methods: {
getBooks() {
const uri = '/api/books.json'

試しに参考書籍をすべて削除してみたところ、期待通り表示がされました!

👇最後の1冊を表示すると、Railsプログラマーコースの書籍一覧に「登録されている本はありません」と表示される
Image from Gyazo

👇既存の書籍一覧でもプラクティスに紐づいていない参考書籍でも登録自体が1冊でも存在する場合、「登録されている本はありません」という表示はされない
Image from Gyazo

title: パーフェクト Ruby on Rails
price: 3637
page_url: https://www.amazon.co.jp/dp/B08D3DW7LP
description: 通称パRails
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.

「変更確認方法」に不足があり、失礼いたしました><!
以下を追記いたしました🙇‍♀️

  1. rails db:seedで本PRで追加した書籍のデータを development 環境に読み込む

たびたびお手数ですが次回確認時にこちらをお試しいただけますでしょうか🙏

@2525nicole 2525nicole force-pushed the feature/create-book-list-for-each-course branch from f8a53c2 to 89f223b Compare December 21, 2023 04:16
@2525nicole
Copy link
Contributor Author

@natsuto6
お忙しいところ早速ご確認をいただきありがとうございます🙇‍♀️!!

リロード時や、「全て」タブから「必読」タブに切り替えた際などに、「プラクティスで絞り込む」のプルダウンが常に全プラクティスの参考書籍を表示に戻ってしまいますが想定通りでしょうか?
そうでない場合、ユーザーが選択したプラクティスがリロードやタブ切り替え後も保持される方が良いと思いました。

ご意見ありがとうございます!
「プラクティスで絞り込む」のプルダウンを設置する場合、
全て⇄必読やタブの切り替え、ロードを行なってもプルダウンは保持されるのがユーザーにとって親切であると気づくことができました🙏

その一方で(そもそもの部分になってしまうのですが)コース別の書籍一覧でプラクティスごとに絞り込む機能が必要だろうか、という点も含めてチーム開発MTGで改めてご相談した結果、
もともと要件として含まれていない機能であるということから、コース別書籍一覧ページにおいて「プラクティスで絞り込む」機能は含めないことになりました。
仕様についてのメモ

そのためプラクティスでの絞り込みフォーム部分のコードは削除しております。
ご確認をいただいたところ大変申し訳ありません。
また、ユーザー側の視点に立った考え方に気づかせていただきありがとうございます🙇‍♀️勉強になります!

3つの書籍しか登録されていませんが、UIの確認やテストを行うにはもう少し書籍データがあっても良いのかなと思いましたがいかがでしょうか🙏

ご提案ありがとうございます!
たしかに3つの書籍のみ(さらにプラクティスに紐づいているものは2冊のみ)では物足りないですね><!
ご指摘のとおりと感じましたので、以下のとおり書籍を追加いたしました。

  • データベース
    • プラクティスに紐づく書籍を2冊追加(うち1冊が必読)
  • テスト用のfixtures
    • プラクティスに紐づく書籍を1冊追加(必読)

データベースへの2冊追加により、画面上「全て」→「必読」に切り替えた際に2行から1行に正しく並ぶことも確認しやすくなったかと思いますがいかがでしょうか👀
Image from Gyazo

テストについても登録済みの2冊とは異なるプラクティスに紐づく書籍を登録したことで、
test 'show only must-read books'にて「期待どおりコース内のプラクティスの必読書籍が表示される」ことを示せるようになったかと思います。

たびたびお手数をおかけしてしまい申し訳ありませんが、ご都合のよろしいタイミングでご確認をいただけますと幸いです。
よろしくお願いいたします🙏

Copy link
Contributor

@Takuya-Sakai91 Takuya-Sakai91 left a comment

Choose a reason for hiding this comment

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

@2525nicole
仕様についてのご確認、書籍の追加等ありがとうございます!
問題なく確認できました🙌
1点ご確認よろしくお願いします。

this.getPractices()
},
methods: {
getBooks() {
Copy link
Contributor

Choose a reason for hiding this comment

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

フェッチロジックはgetBooksメソッドとgetPracticesメソッドで共通していると思うので別メソッドとして抽出してはいかがでしょうか?🙏

@2525nicole 2525nicole self-assigned this Dec 26, 2023
@2525nicole
Copy link
Contributor Author

@natsuto6
おつかれさまです🤝
お忙しいところ早速のご確認ありがとうございました🙇‍♀️
頂戴したとおりfetchロジックが重複していたので、メソッド化いたしました🙏
たびたびお手数をおかけしますが、ご都合のよろしいタイミングでご確認をいただけますと幸いです!
どうぞよろしくお願いいたします🍀

Copy link
Contributor

@Takuya-Sakai91 Takuya-Sakai91 left a comment

Choose a reason for hiding this comment

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

@2525nicole
お待たせいたしました!
確認できましたのでApproveいたします🙌

@2525nicole
Copy link
Contributor Author

@natsuto6
とんでもないです!!お忙しいところご対応いただきありがとうございます🙇‍♀️🙇‍♀️🙇‍♀️
丁寧に確認いただき、自分では気づけなかった部分に気づくことができ大変勉強になりました🙌
ありがとうございました🙏✨
引き続きどうぞよろしくお願いいたします🍀

@komagata
おつかれさまです!
チームメンバーの方にApproveをいただきましたので、レビューをお願いできますでしょうか🙏

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 2c3299c into main Dec 31, 2023
3 checks passed
@komagata komagata deleted the feature/create-book-list-for-each-course branch December 31, 2023 07:50
@github-actions github-actions bot mentioned this pull request Jan 13, 2024
28 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.

3 participants