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

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

Merged
merged 14 commits into from
Feb 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions app/javascript/components/Report.jsx
Original file line number Diff line number Diff line change
@@ -1,22 +1,13 @@
import React from 'react'
import ListComment from './ListComment'

export default function Report({ report, currentUser }) {
export default function Report({ report, currentUserId, displayUserIcon }) {
return (
<div className={`card-list-item ${report.wip ? 'is-wip' : ''}`}>
<div className="card-list-item__inner">
<div className="card-list-item__user">
<a href={report.user.url} className="card-list-item__user-link">
<span className='["a-user-role", roleClass]'>
<img
className="card-list-item__user-icon a-user-icon"
src={report.user.avatar_url}
title={report.user.login_name}
alt={report.user.login_name}
/>
</span>
</a>
</div>
{displayUserIcon && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

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.

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

<DisplayUserIcon report={report} />
)}
<div className="card-list-item__rows">
<div className="card-list-item__row">
<header className="card-list-item-title">
Expand All @@ -38,7 +29,7 @@ export default function Report({ report, currentUser }) {
{report.title}
</a>
</h2>
{currentUser.id === report.user.id && (
{currentUserId === report.user.id && (
<ReportListItemActions report={report} />
)}
</div>
Expand Down Expand Up @@ -76,6 +67,23 @@ export default function Report({ report, currentUser }) {
)
}

const DisplayUserIcon = ({ report }) => {
return (
<div className="card-list-item__user">
<a href={report.user.url} className="card-list-item__user-link">
<span className='["a-user-role", roleClass]'>
<img
className="card-list-item__user-icon a-user-icon"
src={report.user.avatar_url}
title={report.user.login_name}
alt={report.user.login_name}
/>
</span>
</a>
</div>
)
}

const ReportListItemActions = ({ report }) => {
return (
<div className="card-list-item-title__end">
Expand Down
67 changes: 45 additions & 22 deletions app/javascript/components/Reports.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,37 @@ import LoadingListPlaceholder from './LoadingListPlaceholder'
import Report from './Report'
import Pagination from './Pagination'
import PracticeFilterDropdown from './PracticeFilterDropdown'
import UnconfirmedLink from './UnconfirmedLink'

export default function Reports({ user, currentUser, practices }) {
export default function Reports({ all = false, userId = '', practices = false, unchecked = false, displayUserIcon = true, companyId = '', practiceId = '' }) {
Comment on lines -10 to +11
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.

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

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

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

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

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

  • デフォルト引数を使う
  • 分割代入を使う

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

そうです!

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

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

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

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

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

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('')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

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

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

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

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

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

  • 既存の処理を壊さずに新しい処理を追加するため
  • 元の変数名が間違っていたため

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


useEffect(() => {
setPage(page)
}, [page])

useEffect(() => {
setPracticeId(practiceId)
}, [practiceId])
setUserPracticeId(userPracticeId)
}, [userPracticeId])

const { data, error } = useSWR(
`/api/reports.json?user_id=${user.id}&page=${page}&practice_id=${practiceId}`,
practices
? `/api/reports.json?user_id=${userId}&page=${page}&practice_id=${userPracticeId}`
: unchecked
? `/api/reports/unchecked.json?page=${page}&user_id=${userId}`
: userId !== ''
? `/api/reports.json?page=${page}&user_id=${userId}`
: practiceId !== ''
? `/api/reports.json?page=${page}&practice_id=${practiceId}`
: companyId !== ''
? `/api/reports.json?page=${page}&company_id=${companyId}`
: all === true
? `/api/reports.json?page=${page}&practice_id=${userPracticeId}`
: console.log('data_fetched!'),
Copy link
Contributor Author

@lef237 lef237 Feb 16, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

fetcher
)

Expand All @@ -39,22 +52,26 @@ export default function Reports({ user, currentUser, practices }) {
return (
<>
{data.totalPages === 0 && (
<div className="container is-md">
<PracticeFilterDropdown
practices={practices}
setPracticeId={setPracticeId}
practiceId={practiceId}
/>
<NoReports />
<div>
{practices && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<PracticeFilterDropdown
practices={practices}
setPracticeId={setUserPracticeId}
practiceId={userPracticeId}
/>
)}
<NoReports unchecked={unchecked} />
</div>
)}
{data.totalPages > 0 && (
<div className="container is-md">
<PracticeFilterDropdown
practices={practices}
setPracticeId={setPracticeId}
practiceId={practiceId}
/>
<div>
{practices && (
<PracticeFilterDropdown
practices={practices}
setPracticeId={setUserPracticeId}
practiceId={userPracticeId}
/>
)}
<div className="page-content reports">
{data.totalPages > 1 && (
<Pagination
Expand All @@ -72,12 +89,16 @@ export default function Reports({ user, currentUser, practices }) {
<Report
key={report.id}
report={report}
currentUser={currentUser}
currentUserId={report.currentUserId}
displayUserIcon={displayUserIcon}
/>
)
})}
</div>
</ul>
{unchecked && (
<UnconfirmedLink label={'未チェックの日報を一括で開く'} />
)}
{data.totalPages > 1 && (
<Pagination
sum={data.totalPages * per}
Expand All @@ -94,12 +115,14 @@ export default function Reports({ user, currentUser, practices }) {
)
}

const NoReports = () => {
const NoReports = ({ unchecked }) => {
return (
<div className="o-empty-message">
<div className="o-empty-message__icon">
<i className="fa-regular fa-face-sad-tear" />
<p className="o-empty-message__text">日報はまだありません。</p>
{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></>
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

</div>
</div>
)
Expand Down
1 change: 1 addition & 0 deletions app/views/companies/reports/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ header.page-header
.page-body
.container.is-md
div(data-vue="Reports" data-vue-company-id:number="#{@company.id}")
= react_component('Reports', companyId: @company.id)
2 changes: 1 addition & 1 deletion app/views/current_user/reports/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

.page-body
.container.is-md
div(data-vue="Reports" data-vue-user-id:number="#{current_user.id}")
= react_component('Reports', userId: current_user.id)
- if current_user.reports.present?
.form-actions
ul.form-actions__items
Expand Down
2 changes: 1 addition & 1 deletion app/views/practices/reports/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@

.page-body
.container.is-md
div(data-vue="Reports" data-vue-practice-id:number="#{@practice.id}")
= react_component('Reports', practiceId: @practice.id)
7 changes: 1 addition & 6 deletions app/views/reports/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,5 @@ header.page-header

.page-body
.container.is-md
.page-filter.form
= form_with url: reports_path, local: true, method: 'get'
.form-item.is-inline-md-up
= label_tag :practice_id, 'プラクティスで絞り込む', class: 'a-form-label'
= select_tag :practice_id, options_from_collection_for_select(current_user.practices, :id, :title, selected: params[:practice_id]), include_blank: '全ての日報を表示', onchange: 'this.form.submit()', id: 'js-choices-single-select'

div(data-vue="Reports")
= react_component('Reports', all: true, practices: current_user.practices)
4 changes: 2 additions & 2 deletions app/views/reports/show.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@
| 提出物
.side-tabs-contents
.side-tabs-contents__item#side-tabs-content-1
div(data-vue="Reports" data-vue-user-id:number="#{@report.user.id}" data-vue-limit="10" data-vue-display-user-icon:boolean="false")
= react_component('Reports', userId: @report.user.id, displayUserIcon: false)
.side-tabs-contents__item#side-tabs-content-2.is-only-mentor
.user-info
= render 'users/user_secret_attributes', user: @report.user
Expand All @@ -174,7 +174,7 @@
.o-empty-message__text
| 提出物はまだありません。
- else
div(data-vue="Reports" data-vue-user-id:number="#{@report.user.id}" data-vue-limit="10" data-vue-display-user-icon:boolean="false")
= react_component('Reports', userId: @report.user.id, displayUserIcon: false)

- if flash[:notify_help] && flash[:celebrate_report_count]
= render '/shared/modal', id: 'modal-notify-help', modal_title: '🎉 おめでとう!', auto_show: true
Expand Down
2 changes: 1 addition & 1 deletion app/views/reports/unchecked/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ header.page-header

.page-body
.container.is-md
div(data-vue="Reports")
= react_component('Reports', unchecked: true)
2 changes: 1 addition & 1 deletion app/views/talks/show.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,4 @@
class: 'a-button is-sm is-danger is-block'
div(data-vue="UserMentorMemo" data-vue-user-id:number="#{@user.id}" data-vue-products-mode:boolean="#{true}")
.side-tabs-contents__item#side-tabs-content-2
div(data-vue="Reports" data-vue-user-id:number="#{@user.id}" data-vue-limit="10" data-vue-display-user-icon:boolean="false")
= react_component('Reports', userId: @user.id, displayUserIcon: false)
3 changes: 2 additions & 1 deletion app/views/users/reports/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@
= render 'users/page_tabs', user: @user

.page-body
= react_component('Reports', user: @user, currentUser: current_user, practices: @user.practices)
.container.is-md
= react_component('Reports', userId: @user.id, practices: @user.practices)
6 changes: 2 additions & 4 deletions test/system/reports_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -455,10 +455,8 @@ def wait_for_watch_change

test 'display recently reports' do
visit_with_auth report_path(reports(:report10)), 'mentormentaro'
within first('.side-tabs .card-list-item') do
assert_selector 'img[alt="happy"]'
assert_text '今日は頑張りました'
end
assert_selector 'img[alt="happy"]'
assert_text '今日は頑張りました'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end

test 'display list of submission when mentor is access' do
Expand Down