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

提出物個別ページのメンター用サイドカラムに提出物一覧を追加した #5347

Merged
merged 5 commits into from
Aug 27, 2022

Conversation

keiz1213
Copy link
Contributor

@keiz1213 keiz1213 commented Aug 9, 2022

issue

概要

メンターでログインして提出物個別ページにアクセスした時に表示されるサイドカラムに提出物タブを追加した。
_development__OS_X_Mountain_Lionをクリーンインストールする___FBC

備考

  • メンターが日報個別ページにアクセスした際のサイドカラムには提出物タブが有り、こちらを参考にした。
  • ブラウザ幅を狭めるとデザインが一部崩れるがこちらは町田さんに確認済み。
  • 提出物が0の場合はありえないのでその場合は考慮していない。

変更確認方法

  1. feature/add-products-tab-for-mentorをローカルに取り込む
  2. rails sで立ち上げる
  3. mentormentaroでログインする
  4. 提出物一覧から任意の提出物を選び確認する

変更前

_development__OS_X_Mountain_Lionをクリーンインストールする___FBC

変更後

_development__OS_X_Mountain_Lionをクリーンインストールする___FBC

提出物タブをクリックした後

_development__OS_X_Mountain_Lionをクリーンインストールする___FBC

@keiz1213 keiz1213 force-pushed the feature/add-products-tab-for-mentor branch from 6fbb463 to a723fb5 Compare August 16, 2022 09:26
@machida
Copy link
Member

machida commented Aug 17, 2022

@keiz1213 見た目確認しました!このままで大丈夫ですー👍

@machida machida removed their assignment Aug 17, 2022
@keiz1213 keiz1213 marked this pull request as ready for review August 18, 2022 01:53
@keiz1213 keiz1213 requested a review from Nabegon August 18, 2022 02:08
@keiz1213
Copy link
Contributor Author

@Nabegon
お疲れ様です!
こちらレビューをお願いしてもよろしいでしょうか?全然急いでないのでお時間がある時にでもお願いします🙏

@@ -421,13 +421,15 @@ class ProductsTest < ApplicationSystemTestCase
assert_text '直近の日報'
assert_text 'プラクティスメモ'
assert_text 'ユーザーメモ'
assert_selector '#side-tabs-nav-4', text: '提出物'
Copy link
Contributor

Choose a reason for hiding this comment

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

ここだけ、assert_selector '#side-tabs-nav-4', text: '提出物'の書き方をしているのが気になりました!自分の環境でassert_text '提出物'でテストを実行して問題はありませんでした。何か理由がない場合、assert_text '直近の日報'などのようにassert_text '提出物'で書いた方がいいかな?と思ったのですが、いかがでしょうか🙏

students can not see block for mentorsのテストも同様です。

Copy link
Contributor Author

@keiz1213 keiz1213 Aug 24, 2022

Choose a reason for hiding this comment

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

@Nabegon
とても難しそうなissueをやられてる中お時間をさいていただきありがとうございます!🙏

確かにこちらの書き方は不自然だな〜と自分でも思ってました😅
アサーションをassert_text '提出物'ではなくassert_selector '#side-tabs-nav-4', text: '提出物'にした理由としましては、このページにはサイドカラムのタブ以外にも提出物というテキストが存在するため、それとスコープを分ける目的でこのアサーションにしました。

試しに、assert_selector '#side-tabs-nav-4', text: '提出物'assert_text '提出物'に変えて、サイドカラムのタブの文言をていしゅつぶつにしてテストしてみたところテストが通ってしまい、テストの趣旨とは外れた結果となってしまいました。

※ 画像をわかりやすい方に取り替えました8/26
_development__OS_X_Mountain_Lionをクリーンインストールする___FBC

Copy link
Contributor

@Nabegon Nabegon Aug 24, 2022

Choose a reason for hiding this comment

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

@keiz1213 さん、ご教示いただきありがとうございます!なるほどです、提出物という文言があるためにassert_text '提出物'だけでは不十分なのですね。なぜassert_selector '#side-tabs-nav-4', text: '提出物'と書いているのかについて理解できました🙏

@Nabegon
Copy link
Contributor

Nabegon commented Aug 24, 2022

@keiz1213 さん、お疲れ様です!時間かかってしまいすみませんでした💦
1点、細かいことなのですがコメントさせてもらいました🙏
ご確認のほどよろしくお願いします😊

Copy link
Contributor

@Nabegon Nabegon left a comment

Choose a reason for hiding this comment

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

テストの書き方の件、ご教示いただきありがとうございました!Approveします🙆🙆

@keiz1213
Copy link
Contributor Author

@Nabegon

approveありがとうございます!テストの件、一言書いておくべきでした🙏

@komagata

こちらapproveいただきましたので確認お願いいたします。

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

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

@komagata komagata merged commit 6ee1a63 into main Aug 27, 2022
@komagata komagata deleted the feature/add-products-tab-for-mentor branch August 27, 2022 15:12
@github-actions github-actions bot mentioned this pull request Aug 27, 2022
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants