Merge pull request #5072 from consul/top_bar_adjustments

Fix menu on "wide" small screens

app/assets/stylesheets/foundation_and_overrides.scss

@@ -43,9 +43,7 @@
 @include foundation-table;
 @include foundation-tabs;
 @include foundation-thumbnail;
-@include foundation-title-bar;
 @include foundation-tooltip;
-@include foundation-top-bar;
 @include foundation-visibility-classes;
 @include foundation-float-classes;
 @include motion-ui-transitions;

app/assets/stylesheets/layout.scss

@@ -514,26 +514,6 @@ body > header,
     border-bottom: 1px solid #fff;
   }
 
-  .remote-translations-button {
-
-    &.callout {
-      margin: 0;
-      padding: rem-calc(6);
-
-      [type="submit"] {
-        @include brand-color;
-        background: none;
-        border: 0;
-        cursor: pointer;
-        font-weight: bold;
-
-        &:hover {
-          text-decoration: underline;
-        }
-      }
-    }
-  }
-
   .external-links {
     padding: rem-calc(6) 0;
     text-align: center;
@@ -562,90 +542,32 @@ body > header,
 }
 
 .top-bar {
-  background-color: inherit;
+  align-items: center;
-  padding: 0;
+  display: flex;
+  padding-bottom: $topbar-padding;
+  padding-top: $topbar-padding;
 
-  @include breakpoint(medium) {
+  @include breakpoint(small only) {
-    justify-content: flex-start;
+    flex-direction: row-reverse;
+    flex-wrap: wrap;
 
-    .top-bar-title {
+    .responsive-menu {
-      flex-grow: 1;
+      width: 100%;
-    }
-  }
 
-  ul {
+      a {
-    background: none;
+        display: inline-block;
-  }
-
-  .menu > li {
-    display: block;
-    width: 100%;
-
-    @include breakpoint(medium) {
-      width: auto;
-    }
-
-    a {
-      color: inherit;
-      line-height: inherit;
-      padding-left: 0;
-
-      @include breakpoint(medium) {
-        font-size: $small-font-size;
-        padding: rem-calc(8) rem-calc(16);
       }
     }
 
-    .button {
+    [data-responsive-toggle] {
-      background: none;
+      flex-grow: 1000;
-      text-align: left;
-
-      @include breakpoint(medium) {
-        @include brand-color;
-        background: $body-background;
-        text-align: center;
-      }
     }
   }
 
-  .has-submenu {
+  h1 {
-
+    flex-grow: 1;
-    &.is-active a {
-      font-weight: bold;
-    }
-  }
-
-  .menu {
-
-    @include breakpoint(small only) {
-      border-bottom: 1px solid $border;
-      margin-bottom: $line-height;
-      margin-top: $line-height / 2;
-      padding-bottom: $line-height;
-
-      .submenu {
-        margin-top: 0;
-      }
-    }
-
-    &.is-dropdown-submenu {
-      @extend %body-colors;
-      margin: 0;
-      margin-top: rem-calc(-12);
-      padding: 0;
-      z-index: 9;
-    }
-
-    .is-submenu-item {
-      display: block;
-      height: auto;
-      margin-bottom: 0;
-    }
-  }
-
-  .top-bar-title {
     line-height: $line-height;
-    margin-right: 0;
+    margin-bottom: 0;
 
     a img {
       height: rem-calc(48);
@@ -671,11 +593,6 @@ body > header,
   cursor: inherit;
 }
 
-.title-bar {
-  background: none;
-  float: right;
-}
-
 .dropdown.menu > li {
 
   a {
@@ -731,104 +648,6 @@ body > header,
   }
 }
 
-.subnavigation {
-  display: flex;
-  flex-direction: column;
-
-  @include breakpoint(medium) {
-    @include background-with-text-contrast($subnavigation, subnavigation);
-    flex-direction: row;
-    padding-bottom: 0;
-
-    > :first-child {
-      flex-grow: 1;
-    }
-  }
-
-  ul {
-    list-style-type: none;
-    margin: 0;
-
-    li {
-      @include breakpoint(medium) {
-        display: inline-block;
-        margin-right: rem-calc(20);
-      }
-    }
-  }
-
-  a {
-    color: inherit;
-    display: inline-block;
-    padding-bottom: $line-height / 2;
-    padding-top: $line-height / 2;
-    position: relative;
-    text-align: left;
-    width: 100%;
-
-    @include breakpoint(medium) {
-      display: block;
-      font-weight: bold;
-      width: auto;
-
-      &:hover {
-        @include anchor-color;
-      }
-
-      &.is-active {
-        @include brand-color;
-        border-bottom: 2px solid;
-        margin-bottom: 1px;
-      }
-    }
-
-    &:focus {
-      z-index: 1;
-    }
-  }
-
-  .input-group {
-    padding-top: $line-height / 4;
-
-    @include breakpoint(medium) {
-      margin-bottom: 0;
-    }
-  }
-
-  .input-group-field {
-    margin-bottom: 0 !important;
-  }
-
-  .input-group-button {
-    line-height: $line-height * 1.5;
-    padding-bottom: 0;
-
-    button {
-      @include background-with-text-contrast($border);
-      border: 1px solid #ccc;
-      border-left: 0;
-      height: $line-height * 1.5;
-      line-height: $line-height * 1.5;
-      padding-top: 0;
-    }
-  }
-
-  input {
-    height: $line-height * 1.5 !important;
-    margin-bottom: 0;
-    margin-right: 0;
-    width: 100%;
-  }
-}
-
-.top-bar-right {
-
-  .is-active {
-    font-weight: bold;
-    text-decoration: underline;
-  }
-}
-
 .submenu {
   border-bottom: 1px solid $border;
   clear: both;
@@ -1167,9 +986,7 @@ form {
   }
 
   &.primary {
-    background-color: $info-bg;
+    @include info-colors;
-    border-color: $info-border;
-    color: $color-info;
   }
 
   &.warning {

app/assets/stylesheets/layout/account_menu.scss

@@ -0,0 +1,64 @@
+.account-menu.menu {
+
+  @include breakpoint(small only) {
+    margin-top: $line-height / 2;
+
+    &,
+    .menu {
+      border-bottom: 1px solid $border;
+      margin-bottom: $line-height;
+      padding-bottom: $line-height;
+    }
+  }
+
+  .is-dropdown-submenu {
+    @extend %body-colors;
+    margin: 0;
+    margin-top: rem-calc(-12);
+    padding: 0;
+    z-index: 9;
+  }
+
+  .is-submenu-item {
+    display: block;
+    margin-bottom: 0;
+  }
+
+  li {
+    display: block;
+    width: 100%;
+
+    @include breakpoint(medium) {
+      width: auto;
+    }
+
+    a {
+      color: inherit;
+      line-height: inherit;
+      padding-left: 0;
+
+      @include breakpoint(medium) {
+        font-size: $small-font-size;
+        padding: rem-calc(8) rem-calc(16);
+      }
+    }
+
+    .button {
+      background: none;
+      text-align: left;
+
+      @include breakpoint(medium) {
+        @include brand-color;
+        background: $body-background;
+        text-align: center;
+      }
+    }
+  }
+
+  .has-submenu {
+
+    &.is-active a {
+      font-weight: bold;
+    }
+  }
+}

app/assets/stylesheets/layout/admin_header.scss

@@ -15,30 +15,21 @@
   }
 
   .top-bar {
-    height: auto;
-    padding-top: $line-height / 2;
-
     @include breakpoint(small only) {
 
-      .top-bar-left ul {
+      .account-menu {
-        display: inline-block;
+        border-bottom: 0;
-      }
+        padding-bottom: 0;
-
+        margin-bottom: 0;
-      .top-bar-right {
-
-        > ul {
-          border-bottom: 0;
-          padding-bottom: 0;
-          margin-bottom: 0;
-        }
-
-        .submenu {
-          position: initial;
-        }
 
         a {
           font-weight: normal;
         }
+
+        .is-active {
+          font-weight: bold;
+          text-decoration: underline;
+        }
       }
 
       [class^="icon-"] {
@@ -46,17 +37,16 @@
       }
     }
 
+    @include breakpoint(medium) {
+      padding-bottom: 0.8rem;
+      padding-top: 0.8rem;
+    }
+
     [class^="icon-"] {
       font-size: $base-font-size;
     }
 
     h1 {
-      margin-top: $line-height / 2;
-      margin-bottom: 0;
-
-      @include breakpoint(medium) {
-        margin-top: 0;
-      }
 
       small {
         color: inherit;
@@ -69,21 +59,7 @@
         font-family: "Lato" !important;
         font-size: rem-calc(24);
         font-weight: lighter;
-        line-height: 1;
       }
     }
   }
-
-  .top-bar .menu > li {
-
-    @include breakpoint(medium) {
-      height: auto !important;
-    }
-  }
-
-  .title-bar {
-    color: inherit;
-    position: absolute;
-    right: 12px;
-  }
 }

app/assets/stylesheets/layout/remote_translations_button.scss

@@ -0,0 +1,13 @@
+.remote-translations-button {
+  @include info-colors;
+  font-size: $small-font-size;
+  text-align: center;
+  padding: rem-calc(6);
+
+  [type="submit"] {
+    @include link;
+    background: none;
+    border: 0;
+    font-weight: bold;
+  }
+}

app/assets/stylesheets/layout/subnavigation.scss

@@ -0,0 +1,87 @@
+.subnavigation {
+  display: flex;
+  flex-direction: column;
+
+  @include breakpoint(medium) {
+    @include background-with-text-contrast($subnavigation, subnavigation);
+    flex-direction: row;
+    padding-bottom: 0;
+
+    > :first-child {
+      flex-grow: 1;
+    }
+  }
+
+  ul {
+    list-style-type: none;
+    margin: 0;
+
+    li {
+      @include breakpoint(medium) {
+        display: inline-block;
+        margin-right: rem-calc(20);
+      }
+    }
+  }
+
+  a {
+    color: inherit;
+    display: inline-block;
+    padding-bottom: $line-height / 2;
+    padding-top: $line-height / 2;
+    position: relative;
+    text-align: left;
+
+    @include breakpoint(medium) {
+      display: block;
+      font-weight: bold;
+
+      &:hover {
+        @include anchor-color;
+      }
+
+      &.is-active {
+        @include brand-color;
+        border-bottom: 2px solid;
+        margin-bottom: 1px;
+      }
+    }
+
+    &:focus {
+      z-index: 1;
+    }
+  }
+
+  .input-group {
+    padding-top: $line-height / 4;
+
+    @include breakpoint(medium) {
+      margin-bottom: 0;
+    }
+  }
+
+  .input-group-field {
+    margin-bottom: 0 !important;
+  }
+
+  .input-group-button {
+    line-height: $line-height * 1.5;
+    padding-bottom: 0;
+
+    button {
+      @include background-with-text-contrast($border);
+      border: 1px solid #ccc;
+      border-left: 0;
+      height: $line-height * 1.5;
+      line-height: $line-height * 1.5;
+      padding-top: 0;
+    }
+  }
+
+  input {
+    height: $line-height * 1.5 !important;
+    margin-bottom: 0;
+    margin-right: 0;
+    width: 100%;
+  }
+}

app/assets/stylesheets/mixins/colors.scss

@@ -94,6 +94,12 @@
   @include text-color($anchor-color-hover, anchor-color-hover);
 }
 
+@mixin info-colors {
+  background-color: $info-bg;
+  border-color: $info-border;
+  color: $color-info;
+}
+
 %brand-background {
   @include brand-background;
 }

app/components/layout/account_menu_component.html.erb

@@ -1,4 +1,4 @@
-<ul class="menu" data-responsive-menu="medium-dropdown">
+<ul class="account-menu menu" data-responsive-menu="medium-dropdown">
   <%= render Layout::AdminLoginItemsComponent.new(user) %>
   <%= render Layout::NotificationItemComponent.new(user) %>
   <%= render Layout::LoginItemsComponent.new(user) %>

app/components/layout/admin_header_component.html.erb

@@ -1,39 +1,24 @@
 <header class="header">
   <div class="top-links">
-    <%= render "shared/locale_switcher" %>
+    <%= render Layout::LocaleSwitcherComponent.new %>
 
     <%= link_to root_path do %>
       <%= t("admin.dashboard.index.back", org: setting["org_name"]) %>
     <% end %>
   </div>
 
-  <div class="expanded row admin-top-bar">
+  <div class="top-bar">
-    <% if show_account_menu? %>
+    <h1>
-      <div class="title-bar" data-responsive-toggle="responsive_menu" data-hide-for="medium">
+      <%= link_to namespace_path do %>
-        <button class="menu-button" type="button" data-toggle="responsive_menu">
+        <%= setting["org_name"] %>
-          <span class="menu-icon"></span>
+        <br><small><%= namespaced_header_title %></small>
-          <span class="title-bar-title"><%= t("application.menu") %></span>
-        </button>
-      </div>
-    <% end %>
-
-    <div class="top-bar">
-      <div class="top-bar-left">
-        <h1>
-          <%= link_to namespace_path do %>
-            <%= setting["org_name"] %>
-            <br><small><%= namespaced_header_title %></small>
-          <% end %>
-        </h1>
-      </div>
-
-      <% if show_account_menu? %>
-        <div id="responsive_menu">
-          <div class="top-bar-right">
-            <%= render Layout::AccountMenuComponent.new(user) %>
-          </div>
-        </div>
       <% end %>
-    </div>
+    </h1>
+
+    <% if show_account_menu? %>
+      <%= render Layout::ResponsiveMenuComponent.new do %>
+        <%= render Layout::AccountMenuComponent.new(user) %>
+      <% end %>
+    <% end %>
   </div>
 </header>

app/components/layout/remote_translations_button_component.html.erb

@@ -1,5 +1,5 @@
-<div class="remote-translations-button callout primary text-center">
+<div class="remote-translations-button">
-  <% if display_remote_translation_button?(remote_translations) %>
+  <% if display_remote_translation_button? %>
     <%= form_tag remote_translations_path do %>
       <%= hidden_field_tag :remote_translations, remote_translations.to_json %>
       <%= t("remote_translations.text") %>

app/components/layout/remote_translations_button_component.rb

@@ -0,0 +1,20 @@
+class Layout::RemoteTranslationsButtonComponent < ApplicationComponent
+  attr_reader :remote_translations
+
+  def initialize(remote_translations)
+    @remote_translations = remote_translations
+  end
+
+  def render?
+    remote_translations.present? &&
+      RemoteTranslations::Microsoft::AvailableLocales.include_locale?(I18n.locale)
+  end
+
+  private
+
+    def display_remote_translation_button?
+      remote_translations.none? do |remote_translation|
+        RemoteTranslation.remote_translation_enqueued?(remote_translation)
+      end
+    end
+end

app/components/layout/responsive_menu_component.html.erb

@@ -0,0 +1,10 @@
+<span data-responsive-toggle="responsive_menu" data-hide-for="medium">
+  <button type="button" class="menu-button" data-toggle>
+    <span class="menu-icon"></span>
+    <%= t("application.menu") %>
+  </button>
+</span>
+
+<div id="responsive_menu" class="responsive-menu">
+  <%= content %>
+</div>

app/components/layout/responsive_menu_component.rb

@@ -0,0 +1,2 @@
+class Layout::ResponsiveMenuComponent < ApplicationComponent
+end

app/helpers/remote_translations_helper.rb

@@ -1,11 +0,0 @@
-module RemoteTranslationsHelper
-  def display_remote_translation_info?(remote_translations, locale)
-    remote_translations.present? && RemoteTranslations::Microsoft::AvailableLocales.include_locale?(locale)
-  end
-
-  def display_remote_translation_button?(remote_translations)
-    remote_translations.none? do |remote_translation|
-      RemoteTranslation.remote_translation_enqueued?(remote_translation)
-    end
-  end
-end

app/views/layouts/_header.html.erb

@@ -1,41 +1,30 @@
 <header>
-  <% if display_remote_translation_info?(@remote_translations, I18n.locale) %>
+  <%= render Layout::RemoteTranslationsButtonComponent.new(@remote_translations) %>
-    <%= render "shared/remote_translations_button", remote_translations: @remote_translations %>
-  <% end %>
 
   <div class="top-links">
-    <%= render "shared/locale_switcher" %>
+    <%= render Layout::LocaleSwitcherComponent.new %>
     <div class="hide-for-small-only">
-      <%= render "shared/top_links" %>
+      <%= render Layout::TopLinksComponent.new %>
     </div>
   </div>
 
   <div class="top-bar">
-    <span data-responsive-toggle="responsive-menu" data-hide-for="medium">
+    <h1>
-      <button type="button" class="menu-button" data-toggle>
-        <span class="menu-icon"></span>
-        <%= t("application.menu") %>
-      </button>
-    </span>
-
-    <h1 class="top-bar-title">
       <%= link_to root_path, accesskey: "0" do %>
         <%= image_tag(image_path_for("logo_header.png"), alt: setting["org_name"]) %>
       <% end %>
     </h1>
 
-    <div id="responsive-menu">
+    <%= render Layout::ResponsiveMenuComponent.new do %>
-      <div class="top-bar-right">
+      <%= render Layout::AccountMenuComponent.new(current_user) %>
-        <%= render Layout::AccountMenuComponent.new(current_user) %>
 
-        <div class="show-for-small-only">
+      <div class="show-for-small-only">
-          <div class="subnavigation subnavigation-with-top-links">
+        <div class="subnavigation subnavigation-with-top-links">
-            <%= render Layout::SubnavigationComponent.new %>
+          <%= render Layout::SubnavigationComponent.new %>
-            <%= render "shared/top_links" %>
+          <%= render Layout::TopLinksComponent.new %>
-          </div>
         </div>
       </div>
-    </div>
+    <% end %>
   </div>
 
   <div id="navigation_bar" class="subnavigation">

app/views/shared/_locale_switcher.html.erb

@@ -1 +0,0 @@
-<%= render Layout::LocaleSwitcherComponent.new %>

app/views/shared/_top_links.html.erb

@@ -1 +0,0 @@
-<%= render Layout::TopLinksComponent.new %>

spec/components/layout/remote_translations_button_component_spec.rb

@@ -0,0 +1,65 @@
+require "rails_helper"
+
+describe Layout::RemoteTranslationsButtonComponent do
+  let(:translations) { [{}] }
+  let(:component) { Layout::RemoteTranslationsButtonComponent.new(translations) }
+
+  before do
+    allow(RemoteTranslations::Microsoft::AvailableLocales).to receive(:available_locales)
+                                                          .and_return(%w[de en es fr pt zh-Hans])
+  end
+
+  context "locale with English as a fallback" do
+    before do
+      allow(I18n.fallbacks).to receive(:[]).and_return([:en])
+      Globalize.set_fallbacks_to_all_available_locales
+    end
+
+    it "displays the text in English" do
+      I18n.with_locale(:de) { render_inline component }
+
+      expect(page).to have_css ".remote-translations-button"
+      expect(page).to have_content "The content of this page is not available in your language"
+    end
+
+    it "displays the text in English with a locale needing parsing" do
+      I18n.with_locale(:"zh-CN") { render_inline component }
+
+      expect(page).to have_css ".remote-translations-button"
+      expect(page).to have_content "The content of this page is not available in your language"
+    end
+  end
+
+  context "locale with Spanish as a fallback" do
+    before do
+      allow(I18n.fallbacks).to receive(:[]).and_return([:es])
+      Globalize.set_fallbacks_to_all_available_locales
+    end
+
+    it "displays the text in Spanish" do
+      I18n.with_locale(:fr) { render_inline component }
+
+      expect(page).to have_css ".remote-translations-button"
+      expect(page).to have_content "El contenido de esta página no está disponible en tu idioma"
+    end
+
+    it "displays the text in Spanish with a locale needing parsing" do
+      I18n.with_locale(:"pt-BR") { render_inline component }
+
+      expect(page).to have_css ".remote-translations-button"
+      expect(page).to have_content "El contenido de esta página no está disponible en tu idioma"
+    end
+  end
+
+  it "is not rendered when the locale isn't included in microsoft translate client" do
+    I18n.with_locale(:nl) { render_inline component }
+
+    expect(page).not_to be_rendered
+  end
+
+  it "is not rendered when there aren't any remote translations" do
+    render_inline Layout::RemoteTranslationsButtonComponent.new([])
+
+    expect(page).not_to be_rendered
+  end
+end

spec/support/common_actions/users.rb

@@ -75,11 +75,11 @@ module Users
   end
 
   def expect_to_be_signed_in
-    expect(find(".top-bar-right")).to have_content "My account"
+    expect(find("#responsive_menu")).to have_content "My account"
   end
 
   def expect_not_to_be_signed_in
-    expect(find(".top-bar-right")).not_to have_content "My account"
+    expect(find("#responsive_menu")).not_to have_content "My account"
   end
 
   def do_login_for(user, management:)

spec/system/remote_translations_spec.rb

@@ -1,62 +0,0 @@
-require "rails_helper"
-
-describe "Remote Translations" do
-  before do
-    Setting["feature.remote_translations"] = true
-    create(:proposal)
-    available_locales_response = %w[de en es fr pt zh-Hans]
-    expect(RemoteTranslations::Microsoft::AvailableLocales).to receive(:available_locales).at_most(2).times.
-                                                            and_return(available_locales_response)
-    allow(Rails.application.secrets).to receive(:microsoft_api_key).and_return("123")
-  end
-
-  describe "Display remote translation button when locale is included in microsoft translate client" do
-    context "with locale that has :en fallback" do
-      before do
-        allow(I18n.fallbacks).to receive(:[]).and_return([:en])
-        Globalize.set_fallbacks_to_all_available_locales
-      end
-
-      scenario "should display text in English" do
-        visit root_path(locale: :de)
-
-        expect(page).to have_css ".remote-translations-button"
-        expect(page).to have_content "The content of this page is not available in your language"
-      end
-
-      scenario "should display text in English after parse key" do
-        visit root_path(locale: :"zh-CN")
-
-        expect(page).to have_css ".remote-translations-button"
-        expect(page).to have_content "The content of this page is not available in your language"
-      end
-    end
-
-    context "with locale that has :es fallback" do
-      before do
-        allow(I18n.fallbacks).to receive(:[]).and_return([:es])
-        Globalize.set_fallbacks_to_all_available_locales
-      end
-
-      scenario "should display text in Spanish" do
-        visit root_path(locale: :fr)
-
-        expect(page).to have_css ".remote-translations-button"
-        expect(page).to have_content "El contenido de esta página no está disponible en tu idioma"
-      end
-
-      scenario "should display text in Spanish after parse key" do
-        visit root_path(locale: :"pt-BR")
-
-        expect(page).to have_css ".remote-translations-button"
-        expect(page).to have_content "El contenido de esta página no está disponible en tu idioma"
-      end
-    end
-  end
-
-  scenario "Not display remote translation button when locale is not included in microsoft translate client" do
-    visit root_path(locale: :nl)
-
-    expect(page).not_to have_css ".remote-translations-button"
-  end
-end