reports.vueをreactのReports.jsxに書き換えた #6236

lef237 · 2023-02-16T07:16:52Z

Issue

reports.vueをreactに書き換える #6083

概要

reports.vueが使われていた箇所を、Reports.jsxに書き換えました。

また、それに伴い依存関係にあるファイルを修正しました。

変更確認方法

feature/reports-vue-to-reactをローカルに取り込む
rails db:resetをする
bin/setupをおこなう
rails sで起動して、komagataさんでログインする
以下のリンク先を順番に確認する（日報一覧が表示されていることを確認する）

http://localhost:3000/current_user/reports
http://localhost:3000/users/253826460/reports
http://localhost:3000/practices/315059988/reports
http://localhost:3000/talks/868847020#latest-comment
- このページのユーザーの日報のタブをクリックします
http://localhost:3000/companies/636488896/reports
http://localhost:3000/reports/unchecked
http://localhost:3000/reports
http://localhost:3000/reports/12168338

以上で動作確認は終了です。

Screenshot

見た目の変化はありません。

上記のリンク先をそれぞれ確認していただければ大丈夫です！

lef237

コードを読み解く際のメモとして、セルフレビューを致しました。
📝←この絵文字で始まるコメントが、セルフレビューになっております。

lef237 · 2023-02-16T07:19:35Z

test/system/reports_test.rb

-      assert_text '今日は頑張りました'
-    end
+    assert_selector 'img[alt="happy"]'
+    assert_text '今日は頑張りました'


📝CSSの構造が変わったためテストを書き換えました

lef237 · 2023-02-16T07:20:41Z

app/javascript/components/Reports.jsx

+      {unchecked
+        ? <><i className="fa-regular fa-smile" /><p className="o-empty-message__text">未チェックの日報はありません</p></>
+        : <><i className="fa-regular fa-sad-tear" /><p className="o-empty-message__text">'日報はまだありません。'</p></>
+      }


📝uncheckedの有無によって文面が変わるように致しました

lef237 · 2023-02-16T07:22:52Z

app/javascript/components/Reports.jsx

-          />
-          <NoReports />
+        <div>
+          {practices && (


📝practicesが存在するときは、このカッコ内の処理をおこなうようになっています

lef237 · 2023-02-16T07:23:26Z

app/javascript/components/Reports.jsx

+    ? `/api/reports.json?page=${page}&company_id=${companyId}`
+    : all === true
+    ? `/api/reports.json?page=${page}&practice_id=${userPracticeId}`
+    : console.log('data_fetched!'),


📝Reactへ渡される引数によって、fetch先を変えています。

複数の条件に合う引数が渡されることはありますか？
引数の内容がuserId !== '' と all === true の場合に、後ろに書いてあるall === true の処理が実行されないことを懸念しています。
処理実行の優先度など含め問題なさそうであればOKです！

複数の条件に合う引数が渡されることはありますか？

複数の条件に合う引数が渡されることはありません。

slimを見て頂けると分かりやすいと思うのですが、例えば「全ての日報のデータをfetchしたい場合」と、「個人の日報のデータをfetchしてくる場合」で渡す引数が違うからです。
（fetch部分に関しては、どれか１つだけ引数を選ぶ形になっています）

優先度も考えて三項演算子で処理致しました！　なので、問題ないと思います。

複数の条件に合う引数が渡されることはありません。
優先度も考えて三項演算子で処理致しました！

ありがとうございます！
いただいた内容で懸念点解消されました！ 🙌

lef237 · 2023-02-16T07:25:47Z

app/javascript/components/Reports.jsx

  const per = 20
  const neighbours = 4
  const defaultPage = parseInt(queryString.parse(location.search).page) || 1
  const [page, setPage] = useState(defaultPage)
-  const [practiceId, setPracticeId] = useState('')
+  const [userPracticeId, setUserPracticeId] = useState('')


📝プラクティスそのものの日報一覧と、特定のユーザー日報一覧で表示形式が違うため、処理が異なります。そのため、変数名が被らないように書き換えています。

practiceId, setPracticeIdを削除する必要はあるのでしょうか？
単に変数名を被らないようにするためであれば、削除は不要かと思います！
(元の変数名が適切ではなかったとかですかね...?)

(元の変数名が適切ではなかったとかですかね...?)

理由としては、これに近いです。
正確には、元の変数名だと不具合が生じるため、変数名を変更する必要がありました。

11行目でpracticeIdという変数名を使っています。これは、本来のvueのファイルに準拠した名前です。

これと同じ変数名が、こちらで最近追加されたPull Requestで使われてしまっていました。

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

そのため、もしこの16行目がpracticeId, setPracticeIdのままになっていると、 #6044 のPull Requestで実装した処理が動かなくなってしまいます。

（そのままの状態だと、11行目の変数定義と16行目の変数定義がバッティングして、エラーが出てReactが表示されなくなってしまいます💧）

削除をしているわけではなく、変数名を変更してかぶらないようにすることで、既存の処理を壊すことなく実装することができます。

なので、practiceIdをuserPracticeIdへと変更したのは、削除したわけではなく、

既存の処理を壊さずに新しい処理を追加するため

元の変数名が間違っていたため

という２つの理由からおこなっているものです。

この変更に伴い、この直下の22行目や23行目など、影響範囲のあるところも修正しております。

そのため、もしこの16行目がpracticeId, setPracticeIdのままになっていると、#6044 のPull Requestで実装した処理が動かなくなってしまいます。
削除をしているわけではなく、変数名を変更してかぶらないようにすることで、既存の処理を壊すことなく実装することができます。

なるほどです！
既存処理への影響を懸念していたのですが、こちらで問題なさそうです。😊

lef237 · 2023-02-16T07:26:24Z

app/javascript/components/Report.jsx

-            </span>
-          </a>
-        </div>
+        {displayUserIcon && (


📝displayUserIconがある場合のみ、カッコ内の処理が行われます。

この部分を別コンポーネントにするとわかりやすいかな〜と思いました。

別コンポーネントを作成致しました！
差分はこちらになります。→ 34062a1

lef237 · 2023-02-16T09:16:14Z

@mono-nobe
お疲れ様です！　こちらレビューをお願いしてもよろしいでしょうか？
（ご多忙の際は、遠慮なく言って頂いて大丈夫です！）

mono-nobe · 2023-02-16T15:55:14Z

app/javascript/components/Reports.jsx

-export default function Reports({ user, currentUser, practices }) {
+export default function Reports({ all = false, userId = '', practices = false, unchecked = false, displayUserIcon = true, companyId = '', practiceId = '' }) {


変数の増加が気になります 👀
オブジェクトでまとめて扱うなどは難しそうですか？
よろしければ変数が増えている背景を教えていただきたいです！

おそらくmono-nobeさんが仰っていることは、

export default function Reports (props) { // 中の処理 }

このようにpropsのようなオブジェクトにまとめて、後で取り出したほうがスッキリする、ということですよね？👀
自分も以前はそう思っていたのですが、最近では事情が異なっていることを最近知りました。

https://discord.com/channels/715806612824260640/959777673612251146/1070993776899784704

AntiSatoriさんにこちらのDiscordで教えていただいたのですが、今のReactでは

デフォルト引数を使う

分割代入を使う

ことが推奨されているようです！

そのため、ちょっとコードが長くなっていますが、このような形でコードを書きました。

※引数がもっと増えてきたら、引数ごとに改行を入れることが多いです。今回は引数の数がそこまで多くないので、１行で書きました。

このようにpropsのようなオブジェクトにまとめて、後で取り出したほうがスッキリする、ということですよね？

そうです！

自分も以前はそう思っていたのですが、最近では事情が異なっていることを最近知りました。

参考URLありがとうございます！！！

今のReactでは
デフォルト引数を使う
分割代入を使う
ことが推奨されているようです！

そうだったのですね 😳
たしかに、以下の記事を見てみると現状の形式が一番良さそうですね 👀

[React] 関数コンポーネントのpropsの渡し方と分割代入

mono-nobe · 2023-02-16T16:15:16Z

app/javascript/components/Reports.jsx

+    ? `/api/reports.json?page=${page}&company_id=${companyId}`
+    : all === true
+    ? `/api/reports.json?page=${page}&practice_id=${userPracticeId}`
+    : console.log('data_fetched!'),


複数の条件に合う引数が渡されることはありますか？
引数の内容がuserId !== '' と all === true の場合に、後ろに書いてあるall === true の処理が実行されないことを懸念しています。
処理実行の優先度など含め問題なさそうであればOKです！

mono-nobe · 2023-02-16T16:20:36Z

app/javascript/components/Reports.jsx

  const per = 20
  const neighbours = 4
  const defaultPage = parseInt(queryString.parse(location.search).page) || 1
  const [page, setPage] = useState(defaultPage)
-  const [practiceId, setPracticeId] = useState('')
+  const [userPracticeId, setUserPracticeId] = useState('')


practiceId, setPracticeIdを削除する必要はあるのでしょうか？
単に変数名を被らないようにするためであれば、削除は不要かと思います！
(元の変数名が適切ではなかったとかですかね...?)

mono-nobe · 2023-02-16T16:23:50Z

@lef237
お疲れ様です！
ご対応 & レビュー依頼ありがとうございます。😊

コメントしましたので、ご確認をお願いします！
Reactの知見が全くないため、見当違いな指摘があるかもしれないです... 🙏

lef237 · 2023-02-17T00:00:21Z

@mono-nobe
お疲れ様です！
丁寧にレビューして頂き、ありがとうございます✨

コード上でコメント返しを致しました。ご確認頂けると幸いです🙏

mono-nobe · 2023-02-17T00:50:27Z

@lef237
早速のご対応ありがとうございます！
かなり詳細にコメントいただいたおかげで、理解が深まりました 😊
コード部分についてはLGTMです！✨

1つ気になることがあり、http://localhost:3000/reportsにおけるプラクティスでの絞り込みが機能していないように見えるため、ご確認をお願いします 🙏

絞り込みなし

絞り込みあり

表示されている日報に変化なし

対象の日報

OS X Mountain Lionをクリーンインストールするのタグがないため、絞り込み時に除外されるはず

lef237 · 2023-02-17T01:39:26Z

@mono-nobe
お疲れ様です。ご確認ありがとうございます！

絞り込み機能がうまく働いていないバグを修正致しました！　ご確認頂けると幸いです🙏

ご参考に、修正後の動作確認時のgif画像も添付致します。🖼

mono-nobe · 2023-02-17T02:41:55Z

@lef237
ご対応ありがとうございます！
手元でも正常に動作する & 修正内容も問題ないことを確認しましたので承認します！

lef237 · 2023-02-17T02:45:35Z

@mono-nobe
丁寧にレビューして頂いたおかげで、Reactに関する理解が言語化できて、バグも修正することができました😄

ありがとうございました！🙌

lef237 · 2023-02-17T02:46:37Z

@komagata
お疲れ様です！
メンバーレビューが終わりましたので、ご確認頂けると幸いです。よろしくお願いします🙏

lef237 · 2023-02-18T03:45:55Z

@komagata
ご指摘頂いた箇所 #6236 (comment) を修正致しました。

ご確認頂けると幸いです。よろしくお願いします🙏

komagata

確認させて頂きました。OKです〜🙆‍♂️

lef237 commented Feb 16, 2023

View reviewed changes

lef237 requested review from futa4095 and mono-nobe and removed request for futa4095 February 16, 2023 07:29

lef237 self-assigned this Feb 16, 2023

lef237 changed the title ~~Feature/reports vue to react~~ reports.vueをreactに書き換えた Feb 16, 2023

lef237 changed the title ~~reports.vueをreactに書き換えた~~ reports.vueをreactのReports.jsxに書き換えた Feb 16, 2023

lef237 marked this pull request as ready for review February 16, 2023 09:13

mono-nobe reviewed Feb 16, 2023

View reviewed changes

lef237 force-pushed the feature/reports-vue-to-react branch from 7d48314 to 14e13e4 Compare February 17, 2023 01:26

mono-nobe approved these changes Feb 17, 2023

View reviewed changes

lef237 added 11 commits February 18, 2023 12:32

currentUserIdのデータの取得方法を修正した

56f7b3d

practicesが渡されていないときも画面描写がされるように修正した

bc90fec

Propsにuserが渡されていないときも問題なく表示されるようにした

dcf52c2

slim上でuserIdを渡すようにした

c10fc9c

未チェックの提出物一覧を開けるようにした

6a8af25

未チェックの日報が無いときのviewを整えた

f2aea40

displayUserIconの条件分岐を追加した。デザインを調整して汎用的にした。

33a4cb7

会社ごとの日報を表示できるようにした

2365a83

Reactに渡される引数によってfetch先を条件分岐するようにした

a17a224

CSSの構造が変わったため、テストを更新した

17544ee

プルダウンが反映されるように条件分岐の順番を入れ替えた

62bfb0b

lef237 added 3 commits February 18, 2023 12:32

表記ゆれを改善した

252ec9e

フィルター機能を修正した

926e426

displayUserIconを別コンポーネントに分けた

34062a1

lef237 force-pushed the feature/reports-vue-to-react branch from 14e13e4 to 34062a1 Compare February 18, 2023 03:33

komagata approved these changes Feb 18, 2023

View reviewed changes

komagata merged commit bc11903 into main Feb 18, 2023

komagata deleted the feature/reports-vue-to-react branch February 18, 2023 05:13

github-actions bot mentioned this pull request Feb 18, 2023

Release 2023-02-23 04:02:23 #6241

Merged

10 tasks

lef237 mentioned this pull request Feb 24, 2023

show画面でページネーションが表示されないように修正しました #6261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reports.vueをreactのReports.jsxに書き換えた #6236

reports.vueをreactのReports.jsxに書き換えた #6236

lef237 commented Feb 16, 2023 •

edited

Loading

lef237 left a comment

lef237 Feb 16, 2023

lef237 Feb 16, 2023

lef237 Feb 16, 2023

lef237 Feb 16, 2023 •

edited

Loading

mono-nobe Feb 16, 2023

lef237 Feb 16, 2023

mono-nobe Feb 17, 2023

lef237 Feb 16, 2023

mono-nobe Feb 16, 2023

lef237 Feb 16, 2023

mono-nobe Feb 17, 2023

lef237 Feb 16, 2023

komagata Feb 18, 2023

lef237 Feb 18, 2023

lef237 commented Feb 16, 2023

mono-nobe Feb 16, 2023

lef237 Feb 16, 2023

mono-nobe Feb 17, 2023

mono-nobe Feb 16, 2023

mono-nobe Feb 16, 2023

mono-nobe commented Feb 16, 2023

lef237 commented Feb 17, 2023

mono-nobe commented Feb 17, 2023

lef237 commented Feb 17, 2023

mono-nobe commented Feb 17, 2023

lef237 commented Feb 17, 2023

lef237 commented Feb 17, 2023

lef237 commented Feb 18, 2023

komagata left a comment

		export default function Reports({ user, currentUser, practices }) {
		export default function Reports({ all = false, userId = '', practices = false, unchecked = false, displayUserIcon = true, companyId = '', practiceId = '' }) {

reports.vueをreactのReports.jsxに書き換えた #6236

reports.vueをreactのReports.jsxに書き換えた #6236

Conversation

lef237 commented Feb 16, 2023 • edited Loading

Issue

概要

変更確認方法

Screenshot

lef237 left a 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

lef237 Feb 16, 2023 • 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

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

lef237 commented Feb 16, 2023

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

mono-nobe commented Feb 16, 2023

lef237 commented Feb 17, 2023

mono-nobe commented Feb 17, 2023

絞り込みなし

絞り込みあり

対象の日報

lef237 commented Feb 17, 2023

mono-nobe commented Feb 17, 2023

lef237 commented Feb 17, 2023

lef237 commented Feb 17, 2023

lef237 commented Feb 18, 2023

komagata left a comment

Choose a reason for hiding this comment

lef237 commented Feb 16, 2023 •

edited

Loading

lef237 Feb 16, 2023 •

edited

Loading