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

@@ -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;
   }
 

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");
     }

app/assets/stylesheets/notification_item.scss

@@ -1,18 +1,20 @@
 .notifications {
   position: relative;
 
-  &:hover {
-    text-decoration: none;
+  &.unread-notifications {
+    @include has-fa-icon(bell, solid);
+    @include has-fa-icon(circle, solid, after);
+
+    &::before {
+      @include breakpoint(medium) {
+        margin-right: 0;
+      }
     }
 
-  [class^="icon-"] {
-    font-size: rem-calc(19);
-    vertical-align: middle;
-  }
-
-  .icon-circle {
+    &::after {
       color: #ecf00b;
       font-size: rem-calc(10);
+      margin-right: 0;
       position: absolute;
       left: 12px;
       top: 6px;
@@ -23,3 +25,16 @@
       }
     }
   }
+
+  &.no-notifications {
+    @include has-fa-icon(bell, regular);
+  }
+
+  &::before {
+    font-size: rem-calc(19);
+  }
+
+  &:hover {
+    text-decoration: none;
+  }
+}

app/components/layout/notification_item_component.html.erb

@@ -1,18 +1,10 @@
 <% if user %>
   <li id="notifications">
-    <%= link_to notifications_path, rel: "nofollow",
-                class: "notifications" do %>
+    <%= link_to notifications_path, rel: "nofollow", title: text,
+                class: "notifications #{notifications_class}" do %>
       <span class="show-for-sr">
         <%= t("layouts.header.notification_item.notifications") %>
       </span>
-
-      <% if user.notifications.unread.count > 0 %>
-        <span class="icon-circle" aria-hidden="true"></span>
-        <span class="icon-notification" aria-hidden="true" title="<%= text %>"></span>
-      <% else %>
-        <span class="icon-no-notification" aria-hidden="true" title="<%= text %>"></span>
-      <% end %>
-
       <span class="show-for-small-only"><%= text %></span>
     <% end %>
   </li>

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

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

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
 

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