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化(HTMLに変更) #7651

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

nishitatsu-dev
Copy link
Contributor

@nishitatsu-dev nishitatsu-dev commented Apr 10, 2024

Issue

概要

「みんなのブログ」をVueからHTMLに変更しました

変更確認方法

  1. feature/rewrite-external-entries-from-vue-to-html をローカルに取り込む
  2. ログインして、「みんなのブログhttp://localhost:3000/external_entries)」にアクセスする
  3. 以下を確認する
    • 表示が「Vueバージョン」の時と同じであること
    • ページネーションの動作が問題ないこと
    • リンクの動作が問題ないこと
      ※記事のリンクは、存在しないページへのリンクです。URLがhttp://test(番号)/exampleになっていることを確認して下さい。(参考:db/fixtures/external_entries.yml

Screenshot

(表示上の変更は無い為、省略します)

@nishitatsu-dev nishitatsu-dev marked this pull request as ready for review April 10, 2024 08:59
@nishitatsu-dev nishitatsu-dev self-assigned this Apr 10, 2024
@nishitatsu-dev
Copy link
Contributor Author

@goruchanchan
レビューをお願いしたいのですが、よろしいでしょうか?
全く急いでおりません。よろしくお願いいたします🙏

@goruchanchan
Copy link
Contributor

@nishitatsu-dev
レビュー依頼ありがとうございます!一週間程度を目安に確認いたしますので、今しばらくお待ちください🙇‍♂️

@goruchanchan
Copy link
Contributor

@nishitatsu-dev
お疲れ様です!取り急ぎ動作確認実施しました🙇‍♂️2点ほど確認させて欲しいです🙇‍♂️
左側が今回PR分、右側が main ブランチとなっています。

Image from Gyazo

  • 表示が「Vueバージョン」の時と同じであること

    ページネーションのボタン表示方法が微妙に異なるのは問題なしでしょうか?
    (mainブランチは一番端でそれ以上端方向へのボタンリンクを無効化。本PRでは非表示)

  • ページネーションの動作が問題ないこと

    main ブランチは SPA ぽい振る舞いとなっているように感じているのですが、そこまでの再現は不要という認識でいいでしょうか?

  • リンクの動作が問題ないこと

    ご教授いただいた動作を確認できました!!問題ないと思います!

コードレビューも引き続き実施いたします🙇‍♂️

@nishitatsu-dev
Copy link
Contributor Author

@goruchanchan
お疲れ様です!お忙しい所ありがとうございます。
ご質問いただいた件、以下になります。

  • 表示が「Vueバージョン」の時と同じであること
    ページネーションのボタン表示方法が微妙に異なるのは問題なしでしょうか?

ここは、問題なしです。説明不足ですみません🙇🏻
ページネーション自体のデザインは他と同じになっています。
(参考)相談部屋一覧を非vue化する by a-kuroki-gs · Pull Request #7447 · fjordllc/bootcamp

  • ページネーションの動作が問題ないこと
    main ブランチは SPA ぽい振る舞いとなっているように感じているのですが、そこまでの再現は不要という認識でいいでしょうか?

はい。最近の「非Vue化のissue」と同じで、そこまでは不要と考えています
(参考)上記と同じです。

よろしくお願い致します🙏

@nishitatsu-dev
Copy link
Contributor Author

すみません、間違ってcloseのボタンを押してました🙇🏻

Copy link
Contributor

@goruchanchan goruchanchan left a comment

Choose a reason for hiding this comment

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

@nishitatsu-dev
お疲れ様です!いくつか質問しましたので内容のご確認よろしくお願いします🙏

@@ -1,7 +1,5 @@
# frozen_string_literal: true

class API::ExternalEntriesController < API::BaseController
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

@nishitatsu-dev nishitatsu-dev Apr 14, 2024

Choose a reason for hiding this comment

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

このクラスは、今回の変更で利用されなくなりました。
ただ、以下の2つの理由で残すことにしました。

  1. 類似の非Vue化issueで残していたので(参考:相談部屋一覧を非vue化する by a-kuroki-gs · Pull Request #7447 · fjordllc/bootcamp
  2. Issueの説明文に、「最終的にはHotwireにする予定」とあり、Hotwireを調べてみると、APIを使いそうな事が書いてあったので、API関係のものは残した方が良いと考えました(参考:Hotwireとは?|猫でもわかるHotwire入門 Turbo編
    (正直な所、何をどう使うかは分かってないです。。。😅)

Copy link
Contributor

Choose a reason for hiding this comment

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

情報共有ありがとうございます!背景等理解できました🙇‍♂️

@@ -1,2 +1,2 @@
json.external_entries @external_entries, partial: 'api/external_entries/external_entry', as: :external_entry
json.total_pages @external_entries.total_pages
json.total_pages @external_entries.total_pages if @external_entries.respond_to? :total_pages
Copy link
Contributor

Choose a reason for hiding this comment

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

total_pages メソッドがない場合もあるということでしょうか?

Copy link
Contributor Author

@nishitatsu-dev nishitatsu-dev Apr 14, 2024

Choose a reason for hiding this comment

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

これは、今回、APIコントローラ側で@external_entriesを消した為に、total_pagesメソッドが使えない状態になりました。
(この影響で、http://localhost:3000/api/external_entries.json をブラウザで見た時にエラーが出たので、条件式を追加しました。(参考:/api/talks/index.json.jbuilder))

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、app/controllers/api/external_entries_controller.rb から @external_entriesを削除したためということですね👀勉強になりました!

.container.is-md
= paginate(@external_entries)
.card-list.a-card
= render(@external_entries)
Copy link
Contributor

@goruchanchan goruchanchan Apr 14, 2024

Choose a reason for hiding this comment

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

こちらで app/views/external_entries/_external_entry.html.slim をレンダーしている認識でいます。
こちらの理解不足で申し訳ないのですが、= render(@external_entries) とした場合にapp/views/external_entries/_external_entry.html.slimにレンダーされる仕組みなど教えていただけると嬉しいです🙇‍♂️

個人的には、明示的に部分テンプレートとローカル変数を指定した方がわかりやすい気がしたのですが、いかがでしょうか?

Suggested change
= render(@external_entries)
= render partial: 'external_entry', collection: @external_entries, as: :external_entry

Copy link
Contributor Author

@nishitatsu-dev nishitatsu-dev Apr 14, 2024

Choose a reason for hiding this comment

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

  • 書き方の件、アドバイスいただいた書き方に修正します。
    (ローカル環境がエラーで立ち上がらないので、復旧後に修正し、ご連絡します。取り急ぎ。🙇🏻)

  • 仕組みの件ですが、以下の流れになります。

    1. render partial: 'external_entry', collection: @external_entries, as: :external_entryで、asを省略した場合、「パーシャル名」と同名の「ローカル変数名」が渡されます。
      (参考:ActionView::PartialRendererの、「Rendering a collection of partials」に出てくる具体例の説明文を参考にしました)
    2. 上記から、render partial: 'external_entry', collection: @external_entriesと書けます。
    3. さらに省略記法で、render @external_entriesと書けます。これは、使われるパーシャル名は、コレクションの中にある「モデル名」を参照して決定される為だそうです。
      (参考:5.4 コレクションをレンダリングする < Action View の概要 - Railsガイドの最後の段落に記述があり、参考にしました)

Copy link
Contributor

Choose a reason for hiding this comment

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

丁寧にありがとうございます!とても詳しく仕様確認していて素晴らしいと思いました!とても勉強になりました🙇‍♂️
省略形についてはこちらの理解不足があると思うので komagata さんに判断していただければいいかと思います🙇‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

では、省略形は、いったんこのまま進めます〜

@nishitatsu-dev
Copy link
Contributor Author

@goruchanchan
レビューありがとうございます!!!
すみませんが、ローカル環境がエラーで立ち上がらなくなってしまい、コメントのみ書きました。🙇🏻
復旧後、該当箇所を修正しましたら、ご連絡します。
よろしくお願い致します🙏

Copy link
Contributor

@goruchanchan goruchanchan left a comment

Choose a reason for hiding this comment

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

@nishitatsu-dev
ご回答ありがとうございます!ご回答いただい内容で疑問は解消できました🙇‍♂️
追加で一点コメントしましたので内容のご確認よろしくお願いします🙇‍♂️

@@ -1,2 +1,2 @@
json.external_entries @external_entries, partial: 'api/external_entries/external_entry', as: :external_entry
json.total_pages @external_entries.total_pages
json.total_pages @external_entries.total_pages if @external_entries.respond_to? :total_pages
Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、app/controllers/api/external_entries_controller.rb から @external_entriesを削除したためということですね👀勉強になりました!

.container.is-md
= paginate(@external_entries)
.card-list.a-card
= render(@external_entries)
Copy link
Contributor

Choose a reason for hiding this comment

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

丁寧にありがとうございます!とても詳しく仕様確認していて素晴らしいと思いました!とても勉強になりました🙇‍♂️
省略形についてはこちらの理解不足があると思うので komagata さんに判断していただければいいかと思います🙇‍♂️

.card-list-item__row
.card-list-item-title
h2.card-list-item-title__title
= link_to(external_entry.title, external_entry.url, rel: 'noopener', target: '_blank', class: 'card-list-item-title__link a-text-link')
Copy link
Contributor

Choose a reason for hiding this comment

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

link_to ですが他の場所では () 無しで呼び出しているようなので、足並みを揃えてもいいかもです🙇‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

link_toの部分、()を削除しました〜

@nishitatsu-dev nishitatsu-dev force-pushed the feature/rewrite-external-entries-from-vue-to-html branch from e40c533 to dd6d44d Compare April 23, 2024 15:07
@nishitatsu-dev
Copy link
Contributor Author

@goruchanchan
コメントいただいた箇所を修正しましたので、ご確認お願いします🙏
返信が遅くなってしまいすみません🙇🏻
(紆余曲折の末に、OSを再インストールして、何とかローカル環境が立ち上がるようになりました)

Copy link
Contributor

@goruchanchan goruchanchan left a comment

Choose a reason for hiding this comment

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

@nishitatsu-dev
お疲れ様です!ご対応ありがとうございました!
LGTMと思いますので承認いたします🙇‍♂️

@nishitatsu-dev
Copy link
Contributor Author

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

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

def index
@external_entries = ExternalEntry.all.order(published_at: :desc).page(params[:page])
end
def index; end
Copy link
Member

Choose a reason for hiding this comment

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

indexが不要であればcontrollerもここへのrouteも不要だと思うので削除をお願いします~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

下記を行いました〜

  • api関連のファイル(controllerを1個、viewを2個)削除
  • routesの記述を削除

@nishitatsu-dev nishitatsu-dev force-pushed the feature/rewrite-external-entries-from-vue-to-html branch from dd6d44d to 0e1d02c Compare May 21, 2024 05:37
@nishitatsu-dev
Copy link
Contributor Author

@komagata
コメントいただいた箇所を修正しましたので、ご確認お願いします🙏

また、issueの内容とは関係ない修正ですが、下記も行いました

  • コンフリクト対応( mainブランチの最先端にrebase
  • authorの修正(ローカルPCのOS再インストール時に設定ミスしており、アイコンが表示されないコミットがあった為)

@komagata
Copy link
Member

@nishitatsu-dev conflictの修正をお願い致します~。

nishitatsu-dev and others added 4 commits May 30, 2024 15:26
途中保存。テスト未確認、Vueコンポーネント削除前。

途中保存(書き方修正)

途中保存(vueファイル削除。他コメント)

VueからHTMLに移行(メモあり)
@nishitatsu-dev nishitatsu-dev force-pushed the feature/rewrite-external-entries-from-vue-to-html branch from 0e1d02c to c02cdef Compare May 30, 2024 06:38
@nishitatsu-dev
Copy link
Contributor Author

@komagata
conflictの修正しました〜。ご確認お願いします🙏

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 1bf80e9 into main Jun 7, 2024
2 checks passed
@komagata komagata deleted the feature/rewrite-external-entries-from-vue-to-html branch June 7, 2024 01:23
@github-actions github-actions bot mentioned this pull request Jun 7, 2024
7 tasks
@nishitatsu-dev
Copy link
Contributor Author

@komagata
ありがとうございました〜🙏

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

3 participants