-
Notifications
You must be signed in to change notification settings - Fork 72
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
管理ページの企業一覧を非React化した #8230
管理ページの企業一覧を非React化した #8230
Conversation
@hagiya0121 |
@Ryooo-k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ryooo-k
動作に問題はありませんでした。気になった点をコメントしたので確認お願いします🙏
a.a-button.is-sm.is-secondary.is-icon title="アドバイザーサインアップURL" href=company.adviser_sign_up_url | ||
i.fa-solid.fa-user-plus | ||
td.admin-table__item-value.is-text-align-center | ||
a.a-button.is-sm.is-secondary.is-icon title="アドバイザーサインアップURL" href=company.trainee_sign_up_url | ||
i.fa-solid.fa-user-plus | ||
td.admin-table__item-value.is-text-align-center | ||
a.a-button.is-sm.is-secondary.is-icon title="アドバイザーサインアップURL" href="/admin/companies/#{company.id}/edit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
研修生招待リンク
と編集
のリンクのtitle
がアドバイザーサインアップURL
なのは変だと思いました。
もともとのReactのコード自体が間違っているようです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます!修正時に気づくべきでした😅
titleはリンク先のタイトル名に修正いたします。
a href="/companies/#{company.id}" = company.name | ||
td.admin-table__item-value | ||
img.admin-table__item-logo-image src=company.logo_url | ||
td.admin-table__item-value = company.website | ||
td.admin-table__item-value.is-text-align-center | ||
a.a-button.is-sm.is-secondary.is-icon title="アドバイザーサインアップURL" href=company.adviser_sign_up_url | ||
i.fa-solid.fa-user-plus | ||
td.admin-table__item-value.is-text-align-center | ||
a.a-button.is-sm.is-secondary.is-icon title="アドバイザーサインアップURL" href=company.trainee_sign_up_url | ||
i.fa-solid.fa-user-plus | ||
td.admin-table__item-value.is-text-align-center | ||
a.a-button.is-sm.is-secondary.is-icon title="アドバイザーサインアップURL" href="/admin/companies/#{company.id}/edit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aタグではなくlink_to
を使った方がいいと思います🤔
- @companies.each do |company| | ||
tr.admin-table__item | ||
td.admin-table__item-value.company-name | ||
a href="/companies/#{company.id}" = company.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここと53行目もURLヘルパーメゾッドが使えると思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link_to
に変更します。また"/companies/#{company.id}"
のURIをcompany_path(company)
に変換できるので、これも合わせて修正しておきます。
td.admin-table__item-value.company-name | ||
a href="/companies/#{company.id}" = company.name | ||
td.admin-table__item-value | ||
img.admin-table__item-logo-image src=company.logo_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image_tag
が使えると思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/controllers/api/admin/companies_controller.rb
、app/views/api/admin/companies/index.json.jbuilder
、test/integration/api/companies_test.rb
も削除していいと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます。app/views/api/admin/companies/_company.json.jbuilder
も削除して問題なさそうですので、削除しておきます。
@@ -27,4 +27,29 @@ main.page-main | |||
hr.a-border | |||
.page-body | |||
.container.is-lg | |||
= react_component 'AdminCompanies' | |||
div[data-testid="admin-companies"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reactではないのでdata-testid="admin-companies"
はいらないのかなあと思いました🤔
テストファイルでwithin "[data-testid='admin-companies']" do
の形でスコープを限定していて、消してしまってもテストは通るのですがなぜスコープを限定しているのかはよく分からないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かにdata-testid="admin-companies
がなくてもテストは通りますね🧐このDocsを見てテストを安定させてるのかと理解してましたが、SPAだとあまり気にしなくて良さそうですね。
なのでテストコードと共にdata-testid="admin-companies"
は削除しておきます。
test/system/admin/companies_test.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不要なコメントが残っています。
@hagiya0121 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ryooo-k
修正お疲れ様です!
問題なかったのでApproveします👌
@hagiya0121 @komagata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認させて頂きました。OKです〜🙆♂️
@Ryooo-k 本番で確認しましたー🙆♂️ |
@machida |
Issue
概要
管理ページの企業一覧ページを非React化しました。
views/admin/companies/index.html.slim
で使用されているReactコンポーネントAdminCompanies.jsx
を削除し、view内で完結するようにしました。変更確認方法
chore/company-list-non-vue-react
をローカルに取り込むi.
git fetch origin pull/8230/head:chore/company-list-non-vue-react
ii.
git checkout chore/company-list-non-vue-react
foreman start -f Procfile.dev
でローカルサーバーを立ち上げるkomagata
、パスワードtesttest
でログインするScreenshot
今回の変更に伴う画面変更はありません。