提出物一覧をVue.jsで非同期にした #2280

hogucc · 2021-01-27T03:09:18Z

ref #2178

概要

提出物一覧（全て、未チェック、未返信、自分の担当）をVue.jsで非同期に読み込むようにした

やったこと

提出物一覧の読み込みの非同期化

提出物一覧（全て、未チェック、未返信、自分の担当）をVue.jsで非同期に読み込むようにした
ページャーをvuejs-paginateを用いて実装
- products.vueのページャー関連の処理は通知一覧をVue.jsで非同期にしたPRと実装内容を合わせています。
product_checker.vueをproduct.vueの子コンポーネントに変更
一覧ページ全体が読み込まれるまで「ロード中」の文言を表示するようにした
- 「ロード中」の表示は仮の実装。別issueで正式対応予定。経緯は以下のURL参照
  - 通知一覧をVue.jsで非同期にした #2260 (comment)

既存のテストコードの修正

未チェック、未返信、自分の担当タブ関連のシステムテストをproducts_test.rbから別ファイルに切り出し
- uncheked_controller、self_assigned_controller、not_responded_controllerのテストは元々products_test.rbの1ファイルにまとめられていた。が、他のテストはコントローラーごとにファイルが分かれていること、products_test.rb内のテストが増えて読みづらくなったことから、ファイルを分割したほうが良いと考えた
提出物一覧の一括で開くボタンをクリックするテストを追加
- 一括で開くボタンは今回の対応以前から存在していたが、テストが存在しなかったため追加した

hogucc · 2021-01-31T14:39:10Z

@Chi-rr
お手すきの際にコードレビューをお願いいたします🙇‍♀️
（※ コードレビューは運用上、現在システム開発に参加されている生徒の方にお願いすることになっています。が、今回は特別にご本人からレビュワーの経験を積みたいというお話があったので、レビューを担当していただくことになりました。）

hogucc · 2021-02-08T00:55:54Z

@Chi-rr こちらできれば今週の水曜日までにレビューいただけると助かります💦難しい場合もご連絡いただけるとありがたいです🙏

Chi-rr · 2021-02-08T01:18:50Z

@hogucc
遅くなってしまい申し訳ありません🙇‍♀️🙇‍♀️
本日中に確認させていただきます🙌

Chi-rr

遅くなってしまい申し訳ありません🙇‍♀️レビューいたしました！
全体的に良さそうで、少し気になった点にコメントさせていただきました🤲
またご確認いただけると幸いです！

app/javascript/product.vue

Chi-rr · 2021-02-08T14:45:52Z

app/javascript/products.vue

+          :break-view-text="null"
+        )
+      .thread-list.a-card(v-if="products.length > 0")
+        product(v-for="(product, index) in products"


index利用されていなさそうなので、削除できますかね？？

Suggested change

product(v-for="(product, index) in products"

product(v-for="product in products"

削除しました！

fb44480

Chi-rr · 2021-02-08T14:46:51Z

app/javascript/products.vue

+        json.products.forEach(c => {
+          this.products.push(c);
+        });


cはどういう変数でしたでしょうか？
productやpでもわかりやすいかなと思いました！

productに修正しました！

1b04a9f

Chi-rr · 2021-02-08T14:47:36Z

app/javascript/products.vue

+    window.onpopstate = function(){
+      location.href = location.href
+    }


こちらのlocation.href = location.hrefは、vuejs-paginateの実装上必要なんでしたっけ？👀

はい、そうです！ページャーの2ページ目以降がクリックされたときはpushStateではURLの履歴を追加しているのですが、pushStateはページの読み込みまでは行いません。なので、ブラウザの戻る・進むボタンを押したときにURLは切り替わるけど、ページが遷移しない状態になります。そこで戻る・進むが押されたことをwindow.onpopstateイベントで検知して、現在のページへ画面遷移する（location.href = location.href）ようにしています。
もっと他に良い書き方あるのかもと悩んだのですが、bootcampの他のファイルでもlocation.href = location.hrefを使っているところがあったので、この書き方を選択しました。

pushStateはページの読み込みまでは行いません。なので、ブラウザの戻る・進むボタンを押したときにURLは切り替わるけど、ページが遷移しない状態になります。

戻る・進むが押されたことをwindow.onpopstateイベントで検知して、現在のページへ画面遷移する（location.href = location.href）ようにしています。

なるほどです〜👀 勉強になります🙏

Chi-rr · 2021-02-08T14:48:37Z

test/system/product/not_responded_test.rb

+# frozen_string_literal: true
+
+require 'application_system_test_case'
+require 'minitest/mock'


こちらってなぜrequireしているんでしょうか？？👀

元々products_test.rbにあったテストを別ファイルに分けたときに、そのままコピーしていたのを消し忘れてました💦
削除しました！

f24dcb6

hogucc · 2021-02-10T01:47:29Z

@Chi-rr
レビューいただきありがとうございます！
ご指摘点を修正しましたので、お手すきの際にご確認お願いいたします。

またご指摘以外で以下の点を修正しました。こちらも合わせてご確認お願いいたします🙏

通知一覧をVue.jsで非同期にしたPRとコードを統一するために行った修正
87cce12
5118755
product.vueの不要な}を削除
5634da6

Chi-rr

@hogucc
修正ありがとうございます！
追加分合わせて確認いたしました！LGTMです🙆‍♀️

hogucc · 2021-02-10T08:08:14Z

@Chi-rr
ご確認ありがとうございました✨

@machida
ページャーを作り変えたので以前のページャーの見た目と同じにするために、_pagination.sassとproducts.vueにスタイルの定義を追加したのですが、定義場所や内容に問題がないかご確認いただけないでしょうか。
CSSの追加になるので、machidaさんに確認いただいたほうが良いと思い、ご連絡しました。

※ 以下のPRでも同様の修正をしています。

sano11o1 · 2021-02-10T16:19:44Z

@machida @hogucc
hogucc さんの説明にいくつか補足させていただきますー
まず、デザインに影響のあるファイルは2つあります。
1つ目は、_pagination.sassです。
_pagination.sassは1月ほど前、雑談部屋で私が町田さんの指示を受けながら修正したファイルになります。
町田さんのアドバイス通りに記述したので間違いはないと思われますが、念のため確認していただけると助かります。

2つ目は、products.vue(もしくはnotifications.vue, reports.vue)です。
ここの<style>内に仮のデザインを入れています。
https://github.com/fjordllc/bootcamp/blob/5118755bbd1f9d7c619a50fd5c51830bf23f388a/app/javascript/products.vue#L174-L182
.pager-disableは無効なページャーのボタンにつけるクラスになります。
https://github.com/fjordllc/bootcamp/blob/5118755bbd1f9d7c619a50fd5c51830bf23f388a/app/javascript/products.vue#L16-L19
kaminariのページャーのボタンに寄せるためには、クリックできないボタンは表示させないようにしたかったので、この仮デザインを作成しました。
outline: none; ですが、これがないとページャーのボタンをクリックした後に、ボタンに青い枠が表示されるため、記述しました。

また私とuniversatoさんのPRでは、is-top, is-bottomクラスを外しています。

bootcamp/app/javascript/notifications.vue

Lines 2 to 6 in 6e0df0f

    
           .container(v-if="loaded && notifications.length > 0") 
        
             .pagination 
        
               nav.container 
        
                 pager-top( 
        
                   v-if="totalPages > 1"

雑談部屋で指示を受けながらファイルを修正した際に、is-top, is-bottomは共通化させたいので、外しておいてくださいと言われた記憶があります。(PRの説明にすぐに書いておくべきでした...)
以上補足になりますー
お手隙の際に確認していただけると助かります！

hogucc · 2021-02-11T01:40:06Z

@MashioSano
情報共有ありがとうございます！
is-top, is-bottomクラスは後ほど私も外します！

hogucc · 2021-02-12T09:54:46Z

@machida
ページャーを作り変えたので以前のページャーの見た目と同じにするために、_pagination.sassとproducts.vueにスタイルの定義を追加したのですが、定義場所や内容に問題がないかご確認いただけないでしょうか。
CSSの追加になるので、machidaさんに確認いただいたほうが良いと思い、ご連絡しました。

@machida
リマインドさせていただきます🙏
上記のコメントでCSSのレビューを依頼いたしましたので、お手すきの際にご確認いただければと思います🙇‍♀️
補足として、デザインに影響あるファイルについてSanoさんがこのPR上でmachidaさん宛にコメントをくださっているのでそちらも併せてご確認いただけると助かります！

machida · 2021-02-12T11:26:20Z

@hogucc ご連絡ありがとうございますー！確認します🙋‍♂️

machida · 2021-02-18T02:07:55Z

@hogucc @komagata

こちら確認しました！
LGTMですー

今回、ページャーのスタイルをいじってて、不要なリンクを非表示から、disabled な見た目にして、クリックができない、にしたほうが、ページャーのレイアウトの変更が発生しなくて使い心地がよくなるので（特にSPAの場合）、そのようにしました。このPRの提出物一覧ではそのようにしたのですが、別のページでは従来の表示になるので、それは別PRで動作を統一していきたいと思います。

hogucc · 2021-02-18T06:40:59Z

@machida

今回、ページャーのスタイルをいじってて、不要なリンクを非表示から、disabled な見た目にして、クリックができない、にしたほうが、ページャーのレイアウトの変更が発生しなくて使い心地がよくなるので（特にSPAの場合）、そのようにしました。このPRの提出物一覧ではそのようにしたのですが、別のページでは従来の表示になるので、それは別PRで動作を統一していきたいと思います。

承知しました！ご対応ありがとうございます！

@komagata
お手すきの際にコードレビューをお願いいたします🙇‍♀️

machida · 2021-02-22T06:08:14Z

@hogucc @komagata masterをrebaseして、ページャーのズレが解消されているのを確認しました！こちらでマージお願いします🙏

hogucc · 2021-02-22T10:13:41Z

@machida
ご対応ありがとうございます✨

@komagata
こちらコードレビューをお願いいたします🙇‍♀️

komagata · 2021-02-23T07:46:31Z

app/controllers/api/products/not_responded_controller.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class API::Products::NotRespondedController < ApplicationController


https://github.com/fjordllc/bootcamp/blob/master/test/integration/api/practices_test.rb

こういう感じのAPIのテストがあるといいかもです。

@komagata
以下のコミットでテストを追加しました！ご確認お願いいたします🙏
227c93f

また、テストを追加した際に、APIのコントローラーがAPI::BaseControllerを継承していないことに気づいて修正しました。
55e860e

また、テストを追加した際に、APIのコントローラーがAPI::BaseControllerを継承していないことに気づいて修正しました。

ありがとうございます！

Co-authored-by: Chi-rr <52053239+Chi-rr@users.noreply.github.com>

komagata

確認しました、OKですー🙆‍♂️

APIのテストいい感じです、ありがとうございます〜

JunichiIto · 2021-03-02T23:28:12Z

app/controllers/api/products/not_responded_controller.rb

+class API::Products::NotRespondedController < API::BaseController
+  before_action :require_staff_login
+  def index
+    @products = Product.not_responded_products.list.page(params[:page])


@hogucc @komagata
#2398 の調査をしていて気づいたんですが、今の実装だとたぶん提出日順ではなくcreated_at順に並びますね。（詳しく見てませんがVue.js側でソートとかしてないですよね？）

https://github.com/fjordllc/bootcamp/blob/master/app/models/product.rb#L69

https://github.com/fjordllc/bootcamp/blob/master/app/models/product.rb#L38

そもそも論になりますが、データを絞り込むscope（つまりwhere）と並び順を指定するscope（つまりorder）は別々にしておかないと、ユースケースが限定されて再利用性が低くなります。
なので、今のscopeの設計から見直した方がいいように思いました。

@JunichiIto
cc: @komagata
Vue.js側でのソートは特に行っていないので、created_at順に並びますね💦
scopeも再検討するのがよさそうです。

そもそも「自分の担当」タブができたいまタブの構成自体が運用と合っていない気がするので、そこから考え直したほうが良さそうと思いました。
メンターが割り振られてない提出物をわかりやすくしてほしい · Issue #2005

hogucc force-pushed the make-the-submissions-lists-spa branch from 2f96b4b to be0de02 Compare January 29, 2021 13:45

hogucc marked this pull request as ready for review January 31, 2021 14:27

hogucc requested a review from Chi-rr January 31, 2021 14:29

universato mentioned this pull request Feb 2, 2021

日報一覧(全体・未チェック)をVue.jsで非同期にした #2286

Merged

Chi-rr reviewed Feb 8, 2021

View reviewed changes

hogucc force-pushed the make-the-submissions-lists-spa branch from b129b7b to 5118755 Compare February 9, 2021 14:06

Chi-rr approved these changes Feb 10, 2021

View reviewed changes

hogucc requested a review from machida February 10, 2021 08:08

hogucc force-pushed the make-the-submissions-lists-spa branch from 5118755 to beba624 Compare February 11, 2021 03:14

komagata assigned machida Feb 17, 2021

hogucc force-pushed the make-the-submissions-lists-spa branch from beba624 to 403f3ad Compare February 17, 2021 11:45

hogucc mentioned this pull request Feb 17, 2021

通知一覧をVue.jsで非同期にした #2260

Merged

hogucc requested review from komagata and removed request for machida February 18, 2021 09:04

machida force-pushed the make-the-submissions-lists-spa branch from e394b36 to c19b20c Compare February 22, 2021 06:08

komagata requested changes Feb 23, 2021

View reviewed changes

machida assigned hogucc Feb 24, 2021

hogucc and others added 14 commits February 24, 2021 19:44

提出物一覧の一括で開くボタンのテストを追加

04b4398

提出物のテストをコントローラーごとにファイル分けした

95ed3b1

提出物を削除するテストにwait_for_vuejsを追加

63d90b4

product.vueのマスタッシュ構文にスペースを追加

3276fe0

Co-authored-by: Chi-rr <52053239+Chi-rr@users.noreply.github.com>

product.vueの不要な}を削除

4d17631

products.vueの使用していない変数を削除

be07108

提出物一覧取得APIの返却JSONの変数名を修正

a9ef254

提出物一覧のテストで不要な「require 'minitest/mock'」を削除

e209467

products関連のjsファイル内の、ログインユーザーがメンターかどうかを表す変数名を修正

ad2383a

通知一覧に合わせてページャーのスタイル定義を修正

11ba217

products.vueのis-topとis-bottomクラスを削除

3507fb5

他のページのぺージャーでもスタイルを使い回せるようにvueからsassにcssを移動

fe103a5

提出物一覧関連のAPIをApplicationControllerではなくAPI::BaseControllerを継承するように修正

55e860e

提出物一覧関連のAPIのテストを追加

227c93f

hogucc force-pushed the make-the-submissions-lists-spa branch from a618959 to 227c93f Compare February 27, 2021 13:30

hogucc requested a review from komagata February 27, 2021 14:04

komagata approved these changes Mar 1, 2021

View reviewed changes

komagata merged commit bc0d7dc into master Mar 1, 2021

komagata deleted the make-the-submissions-lists-spa branch March 1, 2021 15:04

github-actions bot mentioned this pull request Mar 1, 2021

Release 2021-03-03 03:26:59 #2382

Merged

14 tasks

gallardo16 mentioned this pull request Mar 2, 2021

未返信の提出物一覧に日付ごとの区切り線を入れた #2367

Closed

JunichiIto reviewed Mar 2, 2021

View reviewed changes

JunichiIto mentioned this pull request Mar 3, 2021

提出物一覧の未返信タブを担当なしタブに変更する + サイドバーには「担当なし」件数を表示する #2407

Closed

tam-03 mentioned this pull request Mar 16, 2021

お知らせ一覧をVue.jsで非同期にした #2450

Merged

misosoup160 mentioned this pull request May 10, 2021

ユーザー一覧ページをVue.jsで非同期にする #2671

Merged

alto-I mentioned this pull request May 11, 2021

検索結果一覧をVue.jsで非同期にする #2664

Closed

alto-I mentioned this pull request May 23, 2021

検索結果一覧をVue.jsで非同期にする #2728

Merged

SeijiNumata mentioned this pull request Jun 16, 2021

watch一覧でwatch解除できるようにする #2848

Merged

aim2bpg mentioned this pull request Feb 25, 2022

プラクティス > Docs一覧をVue.js化する #4293

Merged

djkazunoko mentioned this pull request Apr 19, 2023

メンター用提出物個別ページにある該当ユーザーの直近の提出物の表示をVue.js化した #6374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

提出物一覧をVue.jsで非同期にした #2280

提出物一覧をVue.jsで非同期にした #2280

hogucc commented Jan 27, 2021 •

edited

Loading

hogucc commented Jan 31, 2021

hogucc commented Feb 8, 2021

Chi-rr commented Feb 8, 2021

Chi-rr left a comment •

edited

Loading

Chi-rr Feb 8, 2021

hogucc Feb 10, 2021

Chi-rr Feb 8, 2021

hogucc Feb 10, 2021

Chi-rr Feb 8, 2021

hogucc Feb 10, 2021 •

edited

Loading

Chi-rr Feb 10, 2021

Chi-rr Feb 8, 2021

hogucc Feb 10, 2021

hogucc commented Feb 10, 2021 •

edited

Loading

Chi-rr left a comment

hogucc commented Feb 10, 2021

sano11o1 commented Feb 10, 2021

hogucc commented Feb 11, 2021

hogucc commented Feb 12, 2021

machida commented Feb 12, 2021

machida commented Feb 18, 2021

hogucc commented Feb 18, 2021

machida commented Feb 22, 2021

hogucc commented Feb 22, 2021

komagata Feb 23, 2021

hogucc Feb 27, 2021

komagata Mar 1, 2021

komagata left a comment

JunichiIto Mar 2, 2021 •

edited

Loading

hogucc Mar 3, 2021

	product(v-for="(product, index) in products"
	product(v-for="product in products"

		@@ -0,0 +1,8 @@
		# frozen_string_literal: true

		class API::Products::NotRespondedController < ApplicationController

提出物一覧をVue.jsで非同期にした #2280

提出物一覧をVue.jsで非同期にした #2280

Conversation

hogucc commented Jan 27, 2021 • edited Loading

概要

やったこと

提出物一覧の読み込みの非同期化

既存のテストコードの修正

hogucc commented Jan 31, 2021

hogucc commented Feb 8, 2021

Chi-rr commented Feb 8, 2021

Chi-rr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hogucc Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hogucc commented Feb 10, 2021 • edited Loading

Chi-rr left a comment

Choose a reason for hiding this comment

hogucc commented Feb 10, 2021

sano11o1 commented Feb 10, 2021

hogucc commented Feb 11, 2021

hogucc commented Feb 12, 2021

machida commented Feb 12, 2021

machida commented Feb 18, 2021

hogucc commented Feb 18, 2021

machida commented Feb 22, 2021

hogucc commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

JunichiIto Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hogucc commented Jan 27, 2021 •

edited

Loading

Chi-rr left a comment •

edited

Loading

hogucc Feb 10, 2021 •

edited

Loading

hogucc commented Feb 10, 2021 •

edited

Loading

JunichiIto Mar 2, 2021 •

edited

Loading