-
Notifications
You must be signed in to change notification settings - Fork 71
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
ダッシュボードに自分のプロフィールへのリンクを表示させた #3975
ダッシュボードに自分のプロフィールへのリンクを表示させた #3975
Conversation
d1376a9
to
4ff4b8d
Compare
@machida さん |
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.
@maeda-seina
レビュー大変遅くなってすいません🙇♂️🙇♂️🙇♂️
コード自体はLGTMでした異論ありません!
ただリンクが追加され画面遷移の経路が新しく発生しているので、以下のようなテストが追加されているとより良いかなと感じました🙏
テストの内容は一例ですので、ダッシュボードからプロフィールに遷移できることが確認できるものであれば何でもいいかと思います。
test 'transition to my profile from dashbord' do
visit_with_auth '/', 'kimura'
assert_text 'ダッシュボード'
click_link 'マイプロフィール'
assert_text "kimuraのプロフィール"
end
けどリンクなのでテストしていったらきりがないという考えもありそうなので、テストの追加は必須ではないかなとも思います。その点はおまかせします〜。
テスト追加された場合は再度レビュー依頼していただければと思います🙏
87507b8
to
e9c3dc0
Compare
@ot0m1 さん こちらこそ、お仕事などでお忙しい中ありがとうございます! おっしゃる通り、テストがある方が良さそうだと思いましたので、追加しました!(テストコードの作成ありがとうございます!!そのまま使わせて頂きました🙇♂️) また、このテストを新たに追加する上で、ダッシュボードの「日報作成のリンク」のテストがないことに気づいたので、そのテストも一緒に追加しております🙏 お手数お掛けしますが、再度レビューお願い致します! |
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.
@maeda-seina
LGTMです!
test/system/home_test.rb
Outdated
test 'transition to my profile page from dashboard' do | ||
visit_with_auth '/', 'kimura' | ||
assert_text 'ダッシュボード' | ||
click_link 'マイプロフィール' | ||
assert_text 'kimuraのプロフィール' | ||
end | ||
|
||
test 'transition to daily report creation page from dashboard' do | ||
visit_with_auth '/', 'kimura' | ||
assert_text 'ダッシュボード' | ||
click_link '日報作成' | ||
assert_text '日報作成' | ||
end |
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.
こちら、それぞれのページが個別に表示できるというテストがあるのであれば、ダッシュボードからクリックして遷移するようなテストは不要です〜。
(ページにアクセスするSystemテストは非常に重いため、全ての要素をクリックするテストを用意するとテストの合計時間がとても遅くなってしまうからです)
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.
@komagata さん
お疲れ様です。
こちら、それぞれのページが個別に表示できるというテストがあるのであれば、ダッシュボードからクリックして遷移するようなテストは不要です〜。
こちらの件についてですが既存のテストでカバーされていると判断したため、今回作成したテストは削除させて頂きました。(test/system/home_test.rb:115
はtest/system/users_test.rb:6
でカバーできているため削除、test/system/home_test.rb:122
の方は前者のテスト作成に伴って作成したテストのため削除。)
再度レビューのほどよろしくお願い致します!!
e9c3dc0
to
3adc3ae
Compare
@@ -3,9 +3,6 @@ | |||
li.header-dropdown__item | |||
= link_to root_path, class: 'header-dropdown__item-link' do | |||
| ダッシュボード | |||
li.header-dropdown__item |
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.
これを取るってのは必要なんでしたっけ?
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.
@komagata さん
そちらはmachidaさんがデザインを入れたときに削除された箇所になっています。
なので、そのままにしております。
追記
PRの変更箇所の部分に書いておくべきことでした...
書いておきます!!🙇♂️
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.
@komagata @maeda-seina ダッシュボードにリンクを置いたので、こちらを消しましたー
自分のプロフィールを確認することがあまりないので、メニューからは外しました。
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ですー🙆♂️
issue
概要
変更前
変更後
ダッシュボードに自分のプロフィールへのリンクが表示されている。
デモ
リンクを押すと自分のプロフィール画面に遷移する。(kimuraでログインしています。)