「今日の気分」のアイコンを日報の一覧に表示した #5290

yocaji · 2022-07-30T01:00:27Z

Issue

「今日の気分」のアイコンを日報の一覧に表示する #5063

概要

日報一覧パーツに「今日の気分」のアイコンを表示する。
※表示位置・表示サイズは町田さんによるデザイン前の状態です

日報一覧パーツはvue版とslim版2種類があり、それぞれの利用箇所は以下の通り。

app/javascript/report.vue
- 日報詳細（直近の日報）
  - http://localhost:3000/reports/12168338
- 日報一覧
  - http://localhost:3000/reports
  - http://localhost:3000/reports/unchecked
- 自分の日報一覧
  - http://localhost:3000/current_user/reports
- ユーザーの日報一覧
  - http://localhost:3000/users/253826460/reports
- 企業の日報一覧
  - http://localhost:3000/companies/636488896/reports
- 相談部屋（ユーザーの日報・管理者のみ）
  - http://localhost:3000/talks/842643596
app/views/reports/_report.html.slim
- プラクティスの日報一覧
  - http://localhost:3000/practices/315059988/reports
- ダッシュボード（「2回連続sadのユーザー」のカード）
  - http://localhost:3000

動作確認方法

feature/display-niconico-icons-on-report-list をローカルに取り込む
rails s で起動する
mentormentaro でログインする

`app/javascript/report.vue` による表示箇所の確認

日報詳細画面にアクセスする
- http://localhost:3000/reports/12168338
「直近の日報」に気分のアイコンが表示されていることを確認する

`app/views/reports/_report.html.slim` による表示箇所の確認

プラクティスの日報一覧画面にアクセスする
- http://localhost:3000/practices/315059988/reports
気分のアイコンが表示されていることを確認する

yocaji · 2022-07-30T01:04:36Z

app/javascript/report.vue

@@ -16,6 +16,10 @@
            .a-list-item-badge.is-wip(v-if='report.wip')
              span
                | WIP
+            img.niconico-calendar__emotion-image(


ニコニコカレンダーで使われているクラスを仮置きしてあります

yocaji · 2022-07-30T01:04:59Z

app/views/reports/_report.html.slim

@@ -10,6 +10,7 @@
              .a-list-item-badge.is-wip
                span
                  | WIP
+            img.niconico-calendar__emotion-image src="/images/emotion/#{report.emotion}.svg" alt=report.emotion


ニコニコカレンダーで使われているクラスを仮置きしてあります

yocaji · 2022-07-30T13:02:33Z

@machida
こちらは新しく画面に表示するパーツが増えるIssueのため町田さんへのデザイン依頼が必要になるかと思いますが、チームメンバーのレビューと同時進行でよろしかったでしょうか？

machida · 2022-08-01T06:50:08Z

@yocajii はいー同時進行にしたいと思いますー

yocaji · 2022-08-01T11:55:18Z

@machida
承知いたしました、よろしくお願いいたします！

@daiki0381
こちらのレビューをお願いしたいです🙏
（変更にVue関連ファイルも含まれるのですが、daikiさんなら大丈夫そうかと思って依頼させていただきました）
もしご多用だったり不明点がありましたら気兼ねなくお知らせください！

daiki0381

@yocajii
レビュー依頼ありがとうございます😊
確認し、動作とコード共に問題ありませんでした🙆‍♂️

3点仕様に関して気になったので質問させてください🙏

①watch中の日報は「今日の気分」のアイコンは不要でしょうか？

②ブックマークした日報は「今日の気分」のアイコンは不要でしょうか？

③検索した時に表示される日報は「今日の気分」のアイコンは不要でしょうか？

【付けた方がいい理由】

統一性を持たせるために「今日の気分」のアイコンを付けた方がいい。

【付けない方がいい理由】

今回の機能はメンターが今日の気分をひと目でわかるようにするのが目的だから、主に受講生が使用するwatch、ブックマーク、検索の日報に「今日の気分」のアイコンは付けない方がいい。

yocaji · 2022-08-02T01:16:39Z

@daiki0381
丁寧にご確認いただきありがとうございます🙏✨

Watch中一覧・ブックマーク一覧・検索結果については、見た目もUIとしての役割も別なので「今日の気分」アイコンを表示する必要はないと考えました。
daikiさんから見て、統一性というのはどこを指してお考えか教えていただけますか？

メンターの方のお考えも伺ってみたいので、聞いてみます。

@machida
#5063 の要件について確認させてください。
Watch中一覧、ブックマーク一覧、検索結果に表示される日報も、「今日の気分」アイコンを表示させる対象になりますでしょうか？
私としては上記でdaikiさんにお伝えした理由で、「今日の気分」アイコンを表示させる必要はないと考えています。

machida · 2022-08-02T01:45:27Z

@yocajii 今回は日報一覧に出すことしか考えてませんでしたー。一旦、日報一覧だけにして、別の部分でも出したいという声が上がったら別Issueにしたいと思いますー。

yocaji · 2022-08-02T02:46:02Z

@machida
ありがとうございます。要望があれば別Issueで対応とのこと、承知いたしました🙆‍♂️

daiki0381

@yocajii
ご確認いただきありがとうございます🙏 問題無いと思うので、approveさせていただきます🙆‍♂️

daikiさんから見て、統一性というのはどこを指してお考えか教えていただけますか？

日報一覧やプラクティスの日報一覧にはアイコンが付いているけど、検索したり、watchした日報にはアイコンが付いていないのは何故？となるような気がしたので、全ての箇所の日報にアイコンを付けることで統一性を持たせるといいかもと思いました。しかし、yocajiiさんの仰る通り、役割が異なるのでここは別機能として捉えることでアイコンは不要だと自分も思いました。

yocaji · 2022-08-02T06:39:39Z

@daiki0381
詳しくお聞かせくださってありがとうございます。
なるほど、今日の気分アイコンが日報のタイトル（ですかね）と結びついたものだと捉えると、そこに一貫性を見出す考え方もありそうですね。
納得しました👍

yocaji · 2022-08-02T06:40:09Z

@komagata　
~~チームメンバーのレビューが通りましたので、お手隙の際にレビューをお願いいたします🙇‍♀️~~
失礼いたしました、町田さんのデザインを反映できましたら改めてご連絡します。

machida · 2022-08-03T04:16:33Z

@yocajii デザイン入れました！コンフリクトが出たので最新のmainをrebaseしておきますー

yocaji · 2022-08-03T11:51:01Z

@machida
ありがとうございます🙏
無事ローカル側にも反映させることができました。

@komagata
チームメンバーのレビューが通り、町田さんのデザインが入りましたので、お手すきの際にご確認のほどお願いいたします🙇‍♂️

komagata · 2022-08-04T08:05:48Z

app/views/reports/_report.html.slim

@@ -10,6 +10,7 @@
              .a-list-item-badge.is-wip
                span
                  | WIP
+            img.niconico-calendar__emotion-image src="/images/emotion/#{report.emotion}.svg" alt=report.emotion


image_tagヘルパーを使う方がいいかもと思いました〜。

コメントありがとうございます🙏✨
image_tag ヘルパーを使うように修正しました。
また、Vueファイルに入れていただいた町田さんのデザインに合わせてimgタグの挿入位置とクラスも修正しました。
お手数ですがご確認のほどお願いいたします🙇‍♂️

komagata · 2022-08-07T05:05:04Z

app/views/reports/_report.html.slim

@@ -12,6 +12,7 @@
                  | WIP
            h2.card-list-item-title__title(itemprop='name')
              = link_to report, itemprop: 'url', class: 'card-list-item-title__link a-text-link js-unconfirmed-link' do
+                = image_tag("/images/emotion/#{report.emotion}.svg", alt: report.emotion, class: 'card-list-item-title__emotion-image')


Suggested change

= image_tag("/images/emotion/#{report.emotion}.svg", alt: report.emotion, class: 'card-list-item-title__emotion-image')

= image_tag "emotion/#{report.emotion}.svg", alt: report.emotion, class: 'card-list-item-title__emotion-image'

image_tagを使えばこれで行けませんでしたっけ？（動かしてないので自信ないですが）

image_tagのsourceはフルパスじゃなくても良かったんですね 👀📝
修正しましたので、お手数ですがご確認のほどお願いいたします🙇‍♂️

komagata

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

komagata · 2022-08-08T04:16:36Z

app/javascript/report.vue

-              ) {{ report.title }}
+              )
+                img.card-list-item-title__emotion-image(
+                  :src='`/images/emotion/${report.emotion}.svg`',


こういうのはcomputedのメソッドにするとわかりやすいかもです〜

こういうところもcomputedの使いどころなんですね 📝✨
修正いたしましたので、ご確認のほどお願いいたします🙏

komagata · 2022-08-10T06:57:11Z

app/javascript/report.vue

-              ) {{ report.title }}
+              )
+                img.card-list-item-title__emotion-image(
+                  :src='[emotionImg]',


この[]ってなんでしょう？
（僕がVueの機能を知らないだけかもですが・・・）

コメントありがとうございます🙏
class属性に複数のclass名を指定するような時は配列の形で記述できるという書き方をそのまま引き継いでしまっていました。
src属性は1つしかありえないということで[]は不要だったので削除しました。
お手数ですがご確認のほどお願いいたします🙇‍♂️

komagata

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

yocaji · 2022-08-14T05:29:07Z

ご確認ありがとうございます！

JunichiIto · 2022-08-18T23:20:52Z

FYI
対応ありがとうございます！ただ、ちょっと気になる点があったので、issueを再オープンしています。
各位、ご確認よろしくお願いします。

#5063

yocaji self-assigned this Jul 30, 2022

yocaji commented Jul 30, 2022

View reviewed changes

yocaji requested a review from daiki0381 August 1, 2022 12:37

daiki0381 reviewed Aug 1, 2022

View reviewed changes

daiki0381 approved these changes Aug 2, 2022

View reviewed changes

yocaji requested review from komagata and removed request for komagata August 2, 2022 06:40

machida self-assigned this Aug 3, 2022

machida force-pushed the feature/display-niconico-icons-on-report-list branch from 30f35e7 to 42c8bd1 Compare August 3, 2022 04:16

machida removed their assignment Aug 3, 2022

yocaji requested a review from komagata August 3, 2022 11:51

komagata reviewed Aug 4, 2022

View reviewed changes

yocaji force-pushed the feature/display-niconico-icons-on-report-list branch from 42c8bd1 to 8e0abb7 Compare August 5, 2022 04:28

komagata reviewed Aug 7, 2022

View reviewed changes

yocaji force-pushed the feature/display-niconico-icons-on-report-list branch from 8e0abb7 to 34b83e9 Compare August 7, 2022 10:08

komagata reviewed Aug 8, 2022

View reviewed changes

yocaji force-pushed the feature/display-niconico-icons-on-report-list branch from 34b83e9 to ade0574 Compare August 8, 2022 05:29

komagata reviewed Aug 10, 2022

View reviewed changes

日報一覧のVueコンポーネント版に気分のアイコン表示を追加した

7b17d28

yocajii and others added 5 commits August 10, 2022 17:39

日報一覧のslimパーツ版に気分のアイコン表示を追加した

53866c5

日報のfixturesのうちemotionがないものにemotionを追加した

00a5c4d

日報一覧のニコニコアイコンのデザイン調整

f2a438d

imgタグの挿入位置および記法を修正した

e49d078

Vueコンポーネント版でcomputedを活用するようにした

d2efecd

yocaji force-pushed the feature/display-niconico-icons-on-report-list branch from ade0574 to d2efecd Compare August 10, 2022 10:15

komagata approved these changes Aug 12, 2022

View reviewed changes

komagata merged commit 84b1152 into main Aug 12, 2022

komagata deleted the feature/display-niconico-icons-on-report-list branch August 12, 2022 08:20

github-actions bot mentioned this pull request Aug 12, 2022

Release 2022-08-18 08:54:40 #5366

Merged

9 tasks

yocaji mentioned this pull request Aug 19, 2022

提出物ページの「直近の日報」に気分のアイコンが表示されるようにした #5391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

「今日の気分」のアイコンを日報の一覧に表示した #5290

「今日の気分」のアイコンを日報の一覧に表示した #5290

yocaji commented Jul 30, 2022

yocaji Jul 30, 2022

yocaji Jul 30, 2022

yocaji commented Jul 30, 2022

machida commented Aug 1, 2022

yocaji commented Aug 1, 2022

daiki0381 left a comment

yocaji commented Aug 2, 2022

machida commented Aug 2, 2022

yocaji commented Aug 2, 2022

daiki0381 left a comment

yocaji commented Aug 2, 2022

yocaji commented Aug 2, 2022 •

edited

Loading

machida commented Aug 3, 2022

yocaji commented Aug 3, 2022

komagata Aug 4, 2022

yocaji Aug 5, 2022

komagata Aug 7, 2022

yocaji Aug 7, 2022

komagata left a comment

komagata Aug 8, 2022

yocaji Aug 8, 2022

komagata Aug 10, 2022

yocaji Aug 10, 2022

komagata left a comment

yocaji commented Aug 14, 2022

JunichiIto commented Aug 18, 2022

	= image_tag("/images/emotion/#{report.emotion}.svg", alt: report.emotion, class: 'card-list-item-title__emotion-image')
	= image_tag "emotion/#{report.emotion}.svg", alt: report.emotion, class: 'card-list-item-title__emotion-image'

「今日の気分」のアイコンを日報の一覧に表示した #5290

「今日の気分」のアイコンを日報の一覧に表示した #5290

Conversation

yocaji commented Jul 30, 2022

Issue

概要

動作確認方法

app/javascript/report.vue による表示箇所の確認

app/views/reports/_report.html.slim による表示箇所の確認

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yocaji commented Jul 30, 2022

machida commented Aug 1, 2022

yocaji commented Aug 1, 2022

daiki0381 left a comment

Choose a reason for hiding this comment

yocaji commented Aug 2, 2022

machida commented Aug 2, 2022

yocaji commented Aug 2, 2022

daiki0381 left a comment

Choose a reason for hiding this comment

yocaji commented Aug 2, 2022

yocaji commented Aug 2, 2022 • edited Loading

machida commented Aug 3, 2022

yocaji commented Aug 3, 2022

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

komagata 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

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

yocaji commented Aug 14, 2022

JunichiIto commented Aug 18, 2022

`app/javascript/report.vue` による表示箇所の確認

`app/views/reports/_report.html.slim` による表示箇所の確認

yocaji commented Aug 2, 2022 •

edited

Loading