-
Notifications
You must be signed in to change notification settings - Fork 75
日報の個別ページにそのユーザーの未チェックの日報数を表示した #8740
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
Changes from all commits
aa7b91c
b4fb497
fa389f3
c43337b
bc548a0
2ce53fb
6827695
93dafb5
ebdf285
e46f3b9
76ec737
ad86a19
7f417f0
adea367
88b84b1
8fc44fd
1ac7588
6f94e8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module UserDecorator | ||
| module ReportStatus | ||
| REPORT_COUNT_LEVELS = { | ||
| 'is-success' => 0..1, | ||
| 'is-primary' => 2..4, | ||
| 'is-warning' => 5..9 | ||
| }.freeze | ||
|
|
||
| LAST_UNCHECKED_REPORT_COUNT = 1 | ||
|
|
||
| def user_report_count_class(count) | ||
| REPORT_COUNT_LEVELS.find { |_, range| range.include?(count) }&.first || 'is-danger' | ||
| end | ||
|
|
||
| def unchecked_report_message(count, user) | ||
| if count.zero? | ||
| "#{user.login_name}さんの日報へ" | ||
| elsif count == LAST_UNCHECKED_REPORT_COUNT | ||
| "#{user.login_name}さんの未チェックの日報はこれで最後です。" | ||
| else | ||
| "#{user.login_name}さんの未チェックの日報が#{count}件あります。" | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| .card-body-main-actions | ||
| +position(relative, 1) | ||
| padding-block: .75rem | ||
| margin-inline: 1rem | ||
| border-radius: 4px | ||
| +media-breakpoint-up(md) | ||
| padding-inline: 2rem | ||
| margin-bottom: 1.5rem | ||
| +media-breakpoint-down(sm) | ||
| padding-inline: .75rem | ||
| margin-bottom: 1rem | ||
| &.is-success | ||
| background-color: #f8fff2 | ||
| border: 1px solid var(--success) | ||
| color: var(--success-text) | ||
| &.is-primary | ||
| border: 1px solid var(--primary) | ||
| background-color: #f5f5ff | ||
| color: var(--primary-text) | ||
| &.is-warning | ||
| border: 1px solid var(--warning) | ||
| background-color: #fff9e7 | ||
| color: var(--warning-text) | ||
| &.is-danger | ||
| border: 1px solid var(--danger) | ||
| background-color: var(--danger-tint) | ||
| color: var(--danger-text) | ||
|
|
||
| .card-body-main-actions__description | ||
| +text-block(.875rem 1.4) | ||
| margin-bottom: .75em | ||
| text-align: center | ||
|
|
||
| .card-body-main-actions__items | ||
| display: flex | ||
| justify-content: center | ||
|
|
||
| .card-body-main-actions__item | ||
| text-align: center | ||
| flex: 1 | ||
| +media-breakpoint-up(md) | ||
| max-width: 16rem | ||
|
|
||
| .card-body-main-actions__item-cancel | ||
| +hover-link-reversal | ||
| color: var(--muted-text) | ||
| font-size: .8125rem | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,16 @@ def delete_unchecked_report_count | |
| Rails.cache.delete 'unchecked_report_count' | ||
| end | ||
|
|
||
| def user_unchecked_report_count(user) | ||
| Rails.cache.fetch "#{user.id}-user_unchecked_report_count" do | ||
| Report.unchecked.not_wip.user(user).count | ||
| end | ||
| end | ||
|
|
||
| def delete_user_unchecked_report_count(user_id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. キャッシュを削除するメソッドがありますが、どこにも使われていないみたいですー
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ご指摘ありがとうございます! |
||
| Rails.cache.delete "#{user_id}-user_unchecked_report_count" | ||
| end | ||
|
|
||
| def unchecked_product_count | ||
| Rails.cache.fetch 'unchecked_product_count' do | ||
| Product.unhibernated_user_products.unchecked.not_wip.count | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ class ReportNotifier | |
| def call(_name, _started, _finished, _unique_id, payload) | ||
| report = payload[:report] | ||
| Cache.delete_unchecked_report_count | ||
| Cache.delete_user_unchecked_report_count(report.user_id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. この日報の通知機能は今回実装する機能とは特に関係がないように思ったのですが、どうでしょうか?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ご指摘ありがとうございます🙇
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. なるほど、モデル名に引っ張られて勘違いしていたみたいです。 |
||
|
|
||
| return unless report.first_public? | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'test_helper' | ||
| require 'active_decorator_test_case' | ||
|
|
||
| module UserDecorator | ||
| class ReportStatusTest < ActiveDecoratorTestCase | ||
| test 'user_report_count_class' do | ||
| assert_equal 'is-success', user_report_count_class(0) | ||
| assert_equal 'is-success', user_report_count_class(1) | ||
|
|
||
| assert_equal 'is-primary', user_report_count_class(2) | ||
| assert_equal 'is-primary', user_report_count_class(4) | ||
|
|
||
| assert_equal 'is-warning', user_report_count_class(5) | ||
| assert_equal 'is-warning', user_report_count_class(9) | ||
|
|
||
| assert_equal 'is-danger', user_report_count_class(10) | ||
| assert_equal 'is-danger', user_report_count_class(100) | ||
| end | ||
|
|
||
| test 'unchecked_report_message' do | ||
| user = users(:hajime) | ||
| assert_equal "#{user.login_name}さんの日報へ", unchecked_report_message(0, user) | ||
| assert_equal "#{user.login_name}さんの未チェックの日報はこれで最後です。", unchecked_report_message(1, user) | ||
| assert_equal "#{user.login_name}さんの未チェックの日報が2件あります。", unchecked_report_message(2, user) | ||
| end | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.
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.
🛠️ Refactor suggestion
アクセシビリティの改善を検討してください
色のみでステータスを示すスタイリングは、色覚に障害のあるユーザーにとってアクセシブルではありません。アイコンやボーダースタイルなどの追加の視覚的手がかりを検討してください。
以下のような改善案を検討してください:
&.is-success background-color: #f8fff2 border: 1px solid var(--success) color: var(--success-text) + &::before + content: "✓" + margin-right: 0.5rem同様に他の状態についても適切なアイコンやインジケーターを追加することをお勧めします。
📝 Committable suggestion
🤖 Prompt for AI Agents
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.
@hirokiej こちらの指摘に対応してください。(問題なければそのようにコメントして議論が決着するところまでやってください)
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.
Uh oh!
There was an error while loading. Please reload this page.
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 さん

こちらのcoderabbitaiの件ですが、提出物の確認部分でも日数だけでなく、色での判断もしています。
今回も件数を表示しているので修正は不要だと感じております。
デザインに関わる部分なので、町田さんの意見をいただけたら幸いです🙇
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.
@hirokiej 色での判断は不要ですー。理由は、提出物は期限があり、その期限にどれくらい迫っているかをすぐに確認できるように色が用意されているが、日報にはそれがないためです。
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.
@machida さん
ご返信ありがとうございます!
提出物のUIの意図も理解できました🙏
それでは、こちら問題なしということで対応いたします🙇
@coderabbitai
こちら、色での判断はしていないので問題ありません。
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.
@hirokiej さん、ご説明ありがとうございます!
machidaさんからの説明もとても参考になりました。提出物と日報でのUI設計の意図の違いがよく理解できました。確かに、こちらの実装では件数が数値として明確に表示されており、色はあくまで視覚的な補助として機能しているため、アクセシビリティの問題はありませんね。
ご対応いただき、ありがとうございました!
(´・ω・`)
✏️ Learnings added