From 8e0972dd330bf9464160b673e4a744897ad736c8 Mon Sep 17 00:00:00 2001
From: Ben Halpern <bendhalpern@gmail.com>
Date: Fri, 26 Jun 2020 17:21:18 -0400
Subject: [PATCH] [deploy] Remove sort links from tags for logged-out (SEO)
 (#8932)

* Remove sort links from tags for logged-out (SEO)

* Fix tests

* Fix tests take 2

* Final test fix

* Read error message more clearly

* Adjust tests

* Final test fix
---
 app/views/articles/tag_index.html.erb         | 80 ++++++++-----------
 app/views/articles/tags/_meta.html.erb        |  2 +-
 spec/requests/stories_index_spec.rb           | 23 ++++++
 .../user_visits_articles_by_tag_spec.rb       | 30 +++----
 4 files changed, 70 insertions(+), 65 deletions(-)

diff --git a/app/views/articles/tag_index.html.erb b/app/views/articles/tag_index.html.erb
index 936877147..88e521ce5 100644
--- a/app/views/articles/tag_index.html.erb
+++ b/app/views/articles/tag_index.html.erb
@@ -56,53 +56,43 @@
           }
         </style>
       <% end %>
-      <div class="on-page-nav-controls" id="on-page-nav-controls">
-        <div class="on-page-nav-label sub-home-nav">
-          <div class="wide-nav-links">
-            <a class="nav-chronofiter-link <%= "selected" if %w[week month year infinity latest].exclude?(params[:timeframe]) %>" href="<%= list_path %>">
-              FEED
-            </a>
-            <span class="separator"></span>
-            <a class="nav-chronofiter-link <%= "selected" if timeframe_check("week") %>" href="<%= list_path %>/top/week">
-              WEEK
-            </a>
-            <a class="nav-chronofiter-link <%= "selected" if timeframe_check("month") %>" href="<%= list_path %>/top/month">
-              MONTH
-            </a>
-            <a class="nav-chronofiter-link <%= "selected" if timeframe_check("year") %>" href="<%= list_path %>/top/year">
-              YEAR
-            </a>
-            <a class="nav-chronofiter-link <%= "selected" if timeframe_check("infinity") %>" href="<%= list_path %>/top/infinity">
-              INFINITY
-            </a>
-            <span class="separator"></span>
-            <a class="nav-chronofiter-link <%= "selected" if timeframe_check("latest") %>" href="<%= list_path %>/latest">
-              LATEST
-            </a>
-          </div>
-          <div class="narrow-nav-select" aria-label="feed-filter-select">
-            <% if ["week", "month", "year", "infinity", "latest"].exclude?(params[:timeframe]) %>
-              <button type="button" id="narrow-feed-butt">&lt;MY <%= community_name %> FEED&gt;</button>
-            <% elsif timeframe_check('week') %>
-              <button type="button" id="narrow-feed-butt">&lt;PAST WEEK&gt;</button>
-            <% elsif timeframe_check('month') %>
-              <button type="button" id="narrow-feed-butt">&lt;PAST MONTH&gt;</button>
-            <% elsif timeframe_check('year') %>
-              <button type="button" id="narrow-feed-butt">&lt;PAST YEAR&gt;</button>
-            <% elsif timeframe_check('infinity') %>
-              <button type="button" id="narrow-feed-butt">&lt;INFINITY&gt;</button>
-            <% elsif timeframe_check('latest') %>
-              <button type="button" id="narrow-feed-butt">&lt;LATEST&gt;</button>
-            <% end %>
-          </div>
-        </div>
-        <button class="on-page-nav-butt on-page-nav-butt-left" id="on-page-nav-butt-left" aria-label="nav-button-left">
-          <img src="<%= asset_path "stack.svg" %>" alt="left-sidebar-nav">
+      <header class="flex items-center p-2 m:p-0 m:pb-2" id="on-page-nav-controls">
+        <button type="button" class="crayons-btn crayons-btn--ghost crayons-btn--icon mr-2 inline-block m:hidden" id="on-page-nav-butt-left" aria-label="nav-button-left">
+          <%= inline_svg_tag("stack.svg", aria: true, width: 24, height: 24, class: "crayons-icon", title: "Left sidebar") %>
         </button>
-        <button class="on-page-nav-butt on-page-nav-butt-right" id="on-page-nav-butt-right" aria-label="nav-button-right">
-          <img src="<%= asset_path "lightning.svg" %>" alt="right-sidebar-nav">
+        <% if user_signed_in? %>
+          <div class="flex-1 flex items-center justify-between">
+            <h2 class="fs-l fw-heavy">Posts</h2>
+            <nav class="crayons-tabs hidden s:flex">
+              <a href="<%= list_path %>" class="crayons-tabs__item <%= "crayons-tabs__item--current" if %w[week month year infinity latest].exclude?(params[:timeframe]) %>">Feed</a>
+
+              <a href="<%= list_path %>/top/week" class="crayons-tabs__item <%= "crayons-tabs__item--current" if timeframe_check("week") %>">Week</a>
+              <a href="<%= list_path %>/top/month" class="crayons-tabs__item <%= "crayons-tabs__item--current" if timeframe_check("month") %>">Month</a>
+              <a href="<%= list_path %>/top/year" class="crayons-tabs__item <%= "crayons-tabs__item--current" if timeframe_check("year") %>">Year</a>
+              <a href="<%= list_path %>/top/infinity" class="crayons-tabs__item <%= "crayons-tabs__item--current" if timeframe_check("infinity") %>">Infinity</a>
+
+              <a href="<%= list_path %>/latest" class="crayons-tabs__item <%= "crayons-tabs__item--current" if timeframe_check("latest") %>">Latest</a>
+            </nav>
+
+            <select class="crayons-select s:hidden ml-2 s:ml-auto w-auto" id="feed-filter-select" aria-label="feed-filter-select">
+              <option value="<%= list_path %>" <% if ["week", "month", "year", "infinity", "latest"].exclude?(params[:timeframe]) %> selected<% end %>>Feed</option>
+              <option value="<%= list_path %>/top/week" <% if timeframe_check("week") %> selected<% end %>>Week</option>
+              <option value="<%= list_path %>/top/month" <% if timeframe_check("month") %> selected<% end %>>Month</option>
+              <option value="<%= list_path %>/top/year" <% if timeframe_check("year") %> selected<% end %>>Year</option>
+              <option value="<%= list_path %>/top/infinity" <% if timeframe_check("infinity") %> selected<% end %>>Infinity</option>
+              <option value="<%= list_path %>/latest" <% if timeframe_check("latest") %> selected<% end %>>Latest</option>
+            </select>
+          </div>
+        <% else %>
+          <div class="flex-1 flex items-center justify-between">
+            <div class="crayons-notice w-100">👋 <a href="/enter">Sign in</a> for the ability sort posts by <strong>top</strong> and  <strong>latest</strong>.</div>
+          </div>
+        <% end %>
+
+        <button type="button" class="crayons-btn crayons-btn--ghost crayons-btn--icon ml-2 inline-block l:hidden" id="on-page-nav-butt-right" aria-label="nav-button-right">
+          <%= inline_svg_tag("lightning.svg", aria: true, width: 24, height: 24, class: "crayons-icon", title: "Right sidebar") %>
         </button>
-      </div>
+      </header>
 
       <div class="substories" id="substories">
         <%# articles/tags/main_feed will iterate on stories with .each_with_index,
diff --git a/app/views/articles/tags/_meta.html.erb b/app/views/articles/tags/_meta.html.erb
index aa112cd44..cc670cce0 100644
--- a/app/views/articles/tags/_meta.html.erb
+++ b/app/views/articles/tags/_meta.html.erb
@@ -1,5 +1,5 @@
 <% title_with_timeframe(
-     page_title: @tag.capitalize,
+     page_title: @tag.capitalize + (@page.present? && @page > 1 ? " Page #{@page}" : "" ),
      timeframe: params[:timeframe],
      content_for: true,
    ) %>
diff --git a/spec/requests/stories_index_spec.rb b/spec/requests/stories_index_spec.rb
index 4f7ec1551..ee9c26426 100644
--- a/spec/requests/stories_index_spec.rb
+++ b/spec/requests/stories_index_spec.rb
@@ -331,6 +331,11 @@ RSpec.describe "StoriesIndex", type: :request do
         sign_in user
       end
 
+      it "shows tags to signed-in users" do
+        get "/t/#{tag.name}"
+        expect(response.body).to include("crayons-tabs__item crayons-tabs__item--current")
+      end
+
       it "has mod-action-button" do
         get "/t/#{tag.name}"
         expect(response.body).to include('<a class="cta mod-action-button"')
@@ -351,6 +356,12 @@ RSpec.describe "StoriesIndex", type: :request do
     context "without user signed in" do
       let(:tag) { create(:tag) }
 
+      it "shows sign-in notice to non-signed-in users" do
+        get "/t/#{tag.name}"
+        expect(response.body).not_to include("crayons-tabs__item crayons-tabs__item--current")
+        expect(response.body).to include("for the ability sort posts by")
+      end
+
       it "does not render pagination" do
         get "/t/#{tag.name}"
         expect(response.body).not_to include('<span class="olderposts-pagenumber">')
@@ -368,6 +379,18 @@ RSpec.describe "StoriesIndex", type: :request do
         expect(response.body).not_to include('<div id="sidebar-wrapper-right"')
       end
 
+      it "renders proper page title for page 1" do
+        create_list(:article, 20, user: user, featured: true, tags: [tag.name], score: 20)
+        get "/t/#{tag.name}/page/1"
+        expect(response.body).to include("<title>#{tag.name.capitalize} - ")
+      end
+
+      it "renders proper page title for page 2" do
+        create_list(:article, 20, user: user, featured: true, tags: [tag.name], score: 20)
+        get "/t/#{tag.name}/page/2"
+        expect(response.body).to include("<title>#{tag.name.capitalize} Page 2 - ")
+      end
+
       it "does not include current page link" do
         create_list(:article, 20, user: user, featured: true, tags: [tag.name], score: 20)
         get "/t/#{tag.name}/page/2"
diff --git a/spec/system/articles/user_visits_articles_by_tag_spec.rb b/spec/system/articles/user_visits_articles_by_tag_spec.rb
index 06a2934ee..29db9092f 100644
--- a/spec/system/articles/user_visits_articles_by_tag_spec.rb
+++ b/spec/system/articles/user_visits_articles_by_tag_spec.rb
@@ -12,7 +12,9 @@ RSpec.describe "User visits articles by tag", type: :system do
 
   context "when user hasn't logged in" do
     context "when 2 articles" do
-      before { visit "/t/javascript" }
+      before do
+        visit "/t/javascript"
+      end
 
       it "shows the header", js: true, stub_elasticsearch: true do
         within("h1") { expect(page).to have_text("javascript") }
@@ -22,14 +24,6 @@ RSpec.describe "User visits articles by tag", type: :system do
         within("h1") { expect(page).to have_button("Follow") }
       end
 
-      it "shows time buttons" do
-        within("#on-page-nav-controls") do
-          expect(page).to have_link("WEEK", href: "/t/javascript/top/week")
-          expect(page).to have_link("INFINITY", href: "/t/javascript/top/infinity")
-          expect(page).to have_link("LATEST", href: "/t/javascript/latest")
-        end
-      end
-
       it "shows correct articles count" do
         expect(page).to have_selector(".crayons-story", count: 2)
       end
@@ -41,16 +35,6 @@ RSpec.describe "User visits articles by tag", type: :system do
           expect(page).not_to have_text(article2.title)
         end
       end
-
-      it "when user clicks 'week'", js: true, stub_elasticsearch: true do
-        click_on "WEEK"
-
-        within("#articles-list") do
-          expect(page).to have_text(article.title)
-          expect(page).not_to have_text(article3.title)
-          expect(page).not_to have_text(article2.title)
-        end
-      end
     end
 
     context "when more articles" do
@@ -75,5 +59,13 @@ RSpec.describe "User visits articles by tag", type: :system do
 
       within("h1") { expect(page).to have_button("Following") }
     end
+
+    it "shows time buttons" do
+      within("#on-page-nav-controls") do
+        expect(page).to have_link("Week", href: "/t/functional/top/week")
+        expect(page).to have_link("Infinity", href: "/t/functional/top/infinity")
+        expect(page).to have_link("Latest", href: "/t/functional/latest")
+      end
+    end
   end
 end