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化した #8055

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

ham-cap
Copy link
Contributor

@ham-cap ham-cap commented Sep 6, 2024

Issue

概要

これまでVueで実装されていた参考書籍一覧のページ(/books)をHTMLによる実装に変更しました。

変更確認方法

  1. chore/change-books-list-from-vue-to-htmlをローカルに取り込む
  2. http://localhost:3000/books にアクセス
  3. 参考書籍の一覧が表示されていることを確認する
image
  1. プラクティスによる書籍の絞り込み機能が動作していることを確認する
    • 例えば「OS X Mountain Lionをクリーンインストールする」を選択すると以下のようになります。
_development__参考書籍___FBC-2
  • なお、非Vue化に伴い、絞り込みの方法を動的なDOMツリーの編集からサーバーへのリクエストによる絞り込みに変更しています。これにより、絞り込み機能を使用するとURLにpractice_idsearch_termsというパラメータが含まれるようになりました。
    このうちsearch_termsについては絞り込みの選択肢をフリーワードで検索する機能のために必要なものとなります。
    たとえば、絞り込みのプルダウンの最上段にある検索窓に「sql」と入力してプラクティスの選択肢を検索したうえで、「sqlの基礎を理解する」を選択して書籍の絞り込みを行った場合のURLと画面表示は以下のようになりますので、併せてご確認いただければと思います。
_development__参考書籍___FBC-3 image

Screenshot

Vueで実装された既存のページをHTML化したものなので、画面の表示そのものに変更はありません。

@ham-cap ham-cap self-assigned this Sep 6, 2024
@ham-cap
Copy link
Contributor Author

ham-cap commented Sep 6, 2024

@kurumadaisuke
お疲れ様です!
こちらのPRについてレビューをお願いできますでしょうか🙏
特に急ぎではありませんのでお手隙の際で大丈夫です🙆‍♂️
お忙しいかと存じますので難しければ遠慮なくおっしゃってください👍

@kurumadaisuke
Copy link
Contributor

@ham-cap
はい!
こちら確認させていただきます🙏
少々お時間いただければっと思いますがよろしくお願いいたします!

Copy link
Contributor

@kurumadaisuke kurumadaisuke left a comment

Choose a reason for hiding this comment

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

遅くなり申し訳ございませんmm
確認させていただき特に問題なさそうでしたのでApproveいたします🙏

@ham-cap ham-cap marked this pull request as ready for review September 14, 2024 07:53
@ham-cap
Copy link
Contributor Author

ham-cap commented Sep 14, 2024

@kurumadaisuke
お忙しい中ご対応いただきありがとうございました✨

@komagata
チームメンバーからApproveいただきましたのでレビューをお願いいたします🙏

a.tag-links__item-link(href="#{practice.path}")
| #{practice.title}
hr.a-border-tint
- if current_user.admin || current_user.mentor
Copy link
Member

Choose a reason for hiding this comment

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

@@ -16,4 +16,27 @@ header.page-header
i.fas.fa-plus
| 参考書籍登録
hr.a-border
div(data-vue="Books" data-vue-is-admin:boolean="#{current_user.admin?}" data-vue-is-mentor:boolean="#{current_user.mentor?}")
div
Copy link
Member

Choose a reason for hiding this comment

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

slimで実装するなら何もないdivは不要かと思います。
デザインも関係するのであれば @machida さんとも連絡をとってみてください。

@ham-cap ham-cap force-pushed the chore/change-books-list-from-vue-to-html branch from 05c4548 to ba126fc Compare October 2, 2024 05:41
@ham-cap
Copy link
Contributor Author

ham-cap commented Oct 2, 2024

@komagata (CC: @machida )
修正いたしましたのでご確認をお願いいたします。
なお、ご指摘いただいた何もないdivについてですが、確認してみたところこのdivが無くてもデザイン崩れ等ありませんでしたので削除させていただきました🙏

@machida
Copy link
Member

machida commented Oct 2, 2024

何もないdivの削除ありがとうございますー🙏

ul.tag-links__items
- book.practices.each do |practice|
li.tag-links__item
a.tag-links__item-link(href="#{practice.path}")
Copy link
Member

Choose a reason for hiding this comment

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

railsのviewではlink_toを使う方がいいと思います。他にも同じような箇所がないか再確認してみてください。

@ham-cap ham-cap force-pushed the chore/change-books-list-from-vue-to-html branch from ba126fc to 24dd050 Compare October 7, 2024 03:04
@ham-cap
Copy link
Contributor Author

ham-cap commented Oct 7, 2024

@komagata
ご指摘いただいたリンクの箇所について修正いたしました!
なお、直接ご指摘いただいたわけではないですが、aタグからlink_toへ置き換える場合と本質的に同じと考え、一箇所imgタグをimage_tagへ置き換えています。
ご確認のほどよろしくお願いいたします🙏

Comment on lines 10 to 13
@books = books
.with_attached_cover
.includes(:practices)
.order(updated_at: :desc, id: :desc)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@books = books
.with_attached_cover
.includes(:practices)
.order(updated_at: :desc, id: :desc)
@books = books
.with_attached_cover
.includes(:practices)
.order(updated_at: :desc, id: :desc)

でよかった気がします〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata
こちら、この位置で揃えないとRubocopのLayout/MultilineMethodCallIndentationというルールに引っかかってしまうのですが、大丈夫でしょうか?👀

Copy link
Member

Choose a reason for hiding this comment

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

すみません、上記だとrubocopに引っかかるんですね。
下記はいかがでしょうか。

Suggested change
@books = books
.with_attached_cover
.includes(:practices)
.order(updated_at: :desc, id: :desc)
@books = books.with_attached_cover
.includes(:practices)
.order(updated_at: :desc, id: :desc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata
こちらの書き方で大丈夫でした🙆

Comment on lines 6 to 9
= link_to book.page_url,
target: '_blank',
rel: 'noopener',
class: 'card-books-item__cover-container' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
= link_to book.page_url,
target: '_blank',
rel: 'noopener',
class: 'card-books-item__cover-container' do
= link_to book.page_url,
target: '_blank',
rel: 'noopener',
class: 'card-books-item__cover-container' do

で行けた気がします。

Comment on lines 34 to 35
p
|#{book.description}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
p
|#{book.description}
p #{book.description}

@ham-cap
Copy link
Contributor Author

ham-cap commented Nov 11, 2024

@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です〜🙆‍♂️

@komagata komagata merged commit a34722a into main Nov 15, 2024
3 checks passed
@komagata komagata deleted the chore/change-books-list-from-vue-to-html branch November 15, 2024 05:22
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.

4 participants