From 4c289f2489797d334ec143eba67eccc13270bc1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 16 Feb 2021 00:30:01 +0100 Subject: [PATCH] Simplify notification item HTML Other than simplifying the view, we can write tests using `click_link`, which makes the tests more robust. Clicking the `.icon-notification` element was causing some tests to fail when defining a window height of 750 pixels in the `admin_budgets` branch. --- app/assets/stylesheets/admin.scss | 4 +- app/assets/stylesheets/mixins.scss | 8 +-- app/assets/stylesheets/notification_item.scss | 51 ++++++++++++------- .../notification_item_component.html.erb | 12 +---- .../layout/notification_item_component.rb | 14 ++++- spec/shared/system/notifiable_in_app.rb | 13 +++-- spec/system/notifications_spec.rb | 4 +- spec/system/proposal_notifications_spec.rb | 17 +++---- 8 files changed, 72 insertions(+), 51 deletions(-) diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index 51a554038..c67de27e5 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -99,7 +99,7 @@ $sidebar-active: #f4fcd0; } } - [class^="icon-"]:not(.icon-circle) { + [class^="icon-"] { font-size: $base-font-size; } } @@ -156,7 +156,7 @@ $sidebar-active: #f4fcd0; } } - .notifications .icon-circle { + .notifications.unread-notifications::after { color: $admin-color; } diff --git a/app/assets/stylesheets/mixins.scss b/app/assets/stylesheets/mixins.scss index bad75babd..f6a5bef0e 100644 --- a/app/assets/stylesheets/mixins.scss +++ b/app/assets/stylesheets/mixins.scss @@ -163,11 +163,11 @@ margin-right: rem-calc(10); } -@mixin has-fa-icon($icon, $style) { - @extend .fa-#{$icon}; +@mixin has-fa-icon($icon, $style, $position: "before") { - &::before { + &::#{$position} { @extend %font-icon; + @extend .fa-#{$icon}:before; @if $style == "solid" { font-weight: bold; @@ -182,7 +182,7 @@ @supports (mask-image: url()) { - &::before { + &::#{$position} { @extend %svg-icon; mask-image: image-url("fontawesome/#{$style}/#{$icon}.svg"); } diff --git a/app/assets/stylesheets/notification_item.scss b/app/assets/stylesheets/notification_item.scss index fd272250a..5d182a7db 100644 --- a/app/assets/stylesheets/notification_item.scss +++ b/app/assets/stylesheets/notification_item.scss @@ -1,25 +1,40 @@ .notifications { position: relative; + &.unread-notifications { + @include has-fa-icon(bell, solid); + @include has-fa-icon(circle, solid, after); + + &::before { + @include breakpoint(medium) { + margin-right: 0; + } + } + + &::after { + color: #ecf00b; + font-size: rem-calc(10); + margin-right: 0; + position: absolute; + left: 12px; + top: 6px; + + @include breakpoint(medium) { + left: auto; + right: 8px; + } + } + } + + &.no-notifications { + @include has-fa-icon(bell, regular); + } + + &::before { + font-size: rem-calc(19); + } + &:hover { text-decoration: none; } - - [class^="icon-"] { - font-size: rem-calc(19); - vertical-align: middle; - } - - .icon-circle { - color: #ecf00b; - font-size: rem-calc(10); - position: absolute; - left: 12px; - top: 6px; - - @include breakpoint(medium) { - left: auto; - right: 8px; - } - } } diff --git a/app/components/layout/notification_item_component.html.erb b/app/components/layout/notification_item_component.html.erb index 2fbae2250..363c2fd2d 100644 --- a/app/components/layout/notification_item_component.html.erb +++ b/app/components/layout/notification_item_component.html.erb @@ -1,18 +1,10 @@ <% if user %>
  • - <%= link_to notifications_path, rel: "nofollow", - class: "notifications" do %> + <%= link_to notifications_path, rel: "nofollow", title: text, + class: "notifications #{notifications_class}" do %> <%= t("layouts.header.notification_item.notifications") %> - - <% if user.notifications.unread.count > 0 %> - - - <% else %> - - <% end %> - <%= text %> <% end %>
    • diff --git a/app/components/layout/notification_item_component.rb b/app/components/layout/notification_item_component.rb index 37be31de1..4c23a2a4a 100644 --- a/app/components/layout/notification_item_component.rb +++ b/app/components/layout/notification_item_component.rb @@ -8,10 +8,22 @@ class Layout::NotificationItemComponent < ApplicationComponent private def text - if user.notifications.unread.count > 0 + if unread_notifications? t("layouts.header.notification_item.new_notifications", count: user.notifications_count) else t("layouts.header.notification_item.no_notifications") end end + + def notifications_class + if unread_notifications? + "unread-notifications" + else + "no-notifications" + end + end + + def unread_notifications? + user.notifications.unread.count > 0 + end end diff --git a/spec/shared/system/notifiable_in_app.rb b/spec/shared/system/notifiable_in_app.rb index afddae9d3..08e30299d 100644 --- a/spec/shared/system/notifiable_in_app.rb +++ b/spec/shared/system/notifiable_in_app.rb @@ -2,13 +2,13 @@ shared_examples "notifiable in-app" do |factory_name| let(:author) { create(:user, :verified) } let!(:notifiable) { create(factory_name, author: author) } - scenario "Notification icon is shown" do + scenario "Notification message is shown" do create(:notification, notifiable: notifiable, user: author) login_as author visit root_path - expect(page).to have_css ".icon-notification" + expect(page).to have_link "You have a new notification" end scenario "A user commented on my notifiable", :js do @@ -16,7 +16,8 @@ shared_examples "notifiable in-app" do |factory_name| login_as author visit root_path - find(".icon-notification").click + + click_link "You have a new notification" expect(page).to have_css ".notification", count: 1 expect(page).to have_content "Someone commented on" @@ -108,7 +109,8 @@ shared_examples "notifiable in-app" do |factory_name| end within("#notifications") do - find(".icon-no-notification").click + click_link "You don't have new notifications" + expect(page).to have_css ".notification", count: 0 end end @@ -130,7 +132,8 @@ shared_examples "notifiable in-app" do |factory_name| end within("#notifications") do - find(".icon-no-notification").click + click_link "You don't have new notifications" + expect(page).to have_css ".notification", count: 0 end end diff --git a/spec/system/notifications_spec.rb b/spec/system/notifications_spec.rb index 186727ad2..9dba969ad 100644 --- a/spec/system/notifications_spec.rb +++ b/spec/system/notifications_spec.rb @@ -103,14 +103,14 @@ describe "Notifications" do visit root_path within("#notifications") do - expect(page).to have_css(".icon-circle") + expect(page).to have_css(".unread-notifications") end click_notifications_icon first(".notification a").click within("#notifications") do - expect(page).not_to have_css(".icon-circle") + expect(page).not_to have_css(".unread-notifications") end end diff --git a/spec/system/proposal_notifications_spec.rb b/spec/system/proposal_notifications_spec.rb index 7a9377962..15f384763 100644 --- a/spec/system/proposal_notifications_spec.rb +++ b/spec/system/proposal_notifications_spec.rb @@ -198,7 +198,7 @@ describe "Proposal Notifications" do login_as user1 visit root_path - find(".icon-notification").click + find(".unread-notifications").click expect(page).to have_css ".notification", count: 1 @@ -210,7 +210,7 @@ describe "Proposal Notifications" do login_as user2 visit root_path - find(".icon-notification").click + find(".unread-notifications").click expect(page).to have_css ".notification", count: 1 @@ -222,7 +222,7 @@ describe "Proposal Notifications" do login_as user3 visit root_path - find(".icon-no-notification").click + click_link "You don't have new notifications" expect(page).to have_css ".notification", count: 0 end @@ -251,7 +251,7 @@ describe "Proposal Notifications" do login_as user1.reload visit root_path - find(".icon-notification").click + click_link "You have a new notification" expect(page).to have_css ".notification", count: 1 @@ -263,7 +263,7 @@ describe "Proposal Notifications" do login_as user2.reload visit root_path - find(".icon-notification").click + click_link "You have a new notification" expect(page).to have_css ".notification", count: 1 @@ -275,7 +275,7 @@ describe "Proposal Notifications" do login_as user3.reload visit root_path - find(".icon-no-notification").click + click_link "You don't have new notifications" expect(page).to have_css ".notification", count: 0 end @@ -303,7 +303,7 @@ describe "Proposal Notifications" do login_as user visit root_path - find(".icon-notification").click + find(".unread-notifications").click expect(page).to have_css ".notification", count: 1 expect(page).to have_content "This resource is not available anymore" @@ -346,8 +346,7 @@ describe "Proposal Notifications" do login_as user.reload visit root_path - within("#notifications") { expect(page).to have_content :all, "You have 3 new notifications" } - find(".icon-notification").click + click_link "You have 3 new notifications" expect(page).to have_css ".notification", count: 3 expect(page).to have_content "There is one new notification on #{proposal.title}", count: 3