From 11c620bf99fdcf292fb167783b0e43c7b6ef1fff Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Mon, 19 Feb 2024 11:58:37 -0800 Subject: [PATCH] Refactor stimulus nav controllers to follow stimulus best practices. (#4422) Remove unused javascript from nav. --- app/assets/stylesheets/modules/header.css | 34 +++---- .../controllers/dropdown_controller.js | 18 ++++ app/javascript/controllers/nav_controller.js | 96 ++++++------------- app/javascript/src/handle-click.js | 16 ---- app/javascript/src/mobile-nav.js | 70 -------------- app/javascript/src/popup-nav.js | 22 ----- app/views/layouts/application.html.erb | 32 ++++--- 7 files changed, 81 insertions(+), 207 deletions(-) create mode 100644 app/javascript/controllers/dropdown_controller.js delete mode 100644 app/javascript/src/handle-click.js delete mode 100644 app/javascript/src/mobile-nav.js delete mode 100644 app/javascript/src/popup-nav.js diff --git a/app/assets/stylesheets/modules/header.css b/app/assets/stylesheets/modules/header.css index dcd4944f3fd..125888ed936 100644 --- a/app/assets/stylesheets/modules/header.css +++ b/app/assets/stylesheets/modules/header.css @@ -185,25 +185,25 @@ border-radius: 22px; } @media (min-width: 1020px) { - .header__popup__nav-links, .home__header__popup__nav-links { + .header__popup__nav-links.hidden { display: none; } - .header__popup__nav-links.is-expanded, .home__header__popup__nav-links.is-expanded { - margin-top: -7px; - display: block; - float: right; - position: absolute; - right: 0; - text-align: right; } - .header__popup__nav-links .header__nav-link, .home__header__popup__nav-links .header__nav-link { - margin-left: 0; - padding-top: 15px; - padding-right: 30px; - padding-bottom: 15px; - display: block; - width: 100%; } } + .header__popup__nav-links { + margin-top: -7px; + display: block; + float: right; + position: absolute; + right: 0; + text-align: right; } + .header__popup__nav-links .header__nav-link { + margin-left: 0; + padding-top: 15px; + padding-right: 30px; + padding-bottom: 15px; + display: block; + width: 100%; } } @media (min-width: 1020px) { - .header__popup__nav-links.is-expanded { + .header__popup__nav-links { width: 180px; border-bottom-right-radius: 3px; border-bottom-left-radius: 3px; @@ -213,7 +213,7 @@ border-bottom: 1px solid rgba(255, 255, 255, 0.3); } } @media (min-width: 1020px) { - .body--index .header__popup__nav-links.is-expanded { + .body--index .header__popup__nav-links { width: 150px; top: 0; padding-top: 74px; diff --git a/app/javascript/controllers/dropdown_controller.js b/app/javascript/controllers/dropdown_controller.js new file mode 100644 index 00000000000..16e4c999ae3 --- /dev/null +++ b/app/javascript/controllers/dropdown_controller.js @@ -0,0 +1,18 @@ +import { Controller } from "@hotwired/stimulus" + +// I tested the stimulus-dropdown component but it has too many deps. +// This mimics the basic stimulus-dropdown api, so we could swap it in later. +export default class extends Controller { + static targets = ["menu"] + + hide(e) { + !this.element.contains(e.target) && + !this.menuTarget.classList.contains("hidden") && + this.menuTarget.classList.add('hidden'); + } + + toggle(e) { + e.preventDefault(); + this.menuTarget.classList.toggle('hidden'); + } +} diff --git a/app/javascript/controllers/nav_controller.js b/app/javascript/controllers/nav_controller.js index e90954b1fe3..ecf639c9ee6 100644 --- a/app/javascript/controllers/nav_controller.js +++ b/app/javascript/controllers/nav_controller.js @@ -2,76 +2,38 @@ import { Controller } from "@hotwired/stimulus" export default class extends Controller { static targets = [ - "dropdownButton", // carrot icon in full size - "dropdown", // full size dropdown - "sandwichIcon", // mobile sandwich icon - "header", - "main", - "footer", - "headerSearch", - "headerLogo" + "collapse", // targets that receive expanded class when mobile nav shows + "header", // target on which clicks don't hide mobile nav + "logo", + "search", ] - - static mobileNavExpandedClass = 'mobile-nav-is-expanded'; - - connect() { - // variable to support mobile nav tab behaviour - // * skipSandwichIcon is for skipping sandwich icon - // when you tab from "gem" icon - // * tabDirection is for hiding and showing navbar - // when you tab in and out - this.skipSandwichIcon = true; - - document.addEventListener("click", (e) => { - // Must check both dropdowns otherwise you always close - // the dropdown because one or the other isn't being clicked - if (!this.dropdownTarget.contains(e.target) && !this.headerTarget.contains(e.target)) { - this.dropdownTarget.classList.remove('is-expanded'); - this.collapseMobileNav(); - } - }); - } - - dropdownButtonTargetConnected(el) { - el.addEventListener("click", (e) => { - e.preventDefault(); - this.dropdownTarget.classList.toggle('is-expanded'); - }); + static classes = ["expanded"] + + connect() { this.skipSandwichIcon = true } + + toggle(e) { + e.preventDefault(); + if (this.collapseTarget.classList.contains(this.expandedClass)) { + this.leave() + this.logoTarget.focus(); + } else { + this.enter() + } } - sandwichIconTargetConnected(el) { - el.addEventListener("click", (e) => { - e.preventDefault(); - - if (this.headerTarget.classList.contains(this.constructor.mobileNavExpandedClass)) { - this.collapseMobileNav(); - } else { - this.expandMobileNav(); - } - }); - - el.addEventListener('focusin', () => { - if (this.skipSandwichIcon) { - this.expandeMobileNav(); - this.headerSearchTarget.focus(); - this.skipSandwichIcon = false; - } else { - this.collapseMobileNav(); - this.headerLogoTarget.focus(); - this.skipSandwichIcon = true; - } - }); + focus() { + if (this.skipSandwichIcon) { // skip sandwich icon when you tab from "gem" icon + this.enter(); + this.hasSearchTarget && this.searchTarget.focus(); + this.skipSandwichIcon = false; + } else { + this.leave(); + this.logoTarget.focus(); + this.skipSandwichIcon = true; + } } - collapseMobileNav() { - this.headerTarget.classList.remove(this.constructor.mobileNavExpandedClass); - this.mainTarget.classList.remove(this.constructor.mobileNavExpandedClass); - this.footerTarget.classList.remove(this.constructor.mobileNavExpandedClass); - } - - expandMobileNav() { - this.headerTarget.classList.add(this.constructor.mobileNavExpandedClass); - this.mainTarget.classList.add(this.constructor.mobileNavExpandedClass); - this.footerTarget.classList.add(this.constructor.mobileNavExpandedClass); - } + hide(e) { !this.headerTarget.contains(e.target) && this.leave() } + leave() { this.collapseTargets.forEach(el => el.classList.remove(this.expandedClass)) } + enter() { this.collapseTargets.forEach(el => el.classList.add(this.expandedClass)) } } diff --git a/app/javascript/src/handle-click.js b/app/javascript/src/handle-click.js deleted file mode 100644 index c1cac780c37..00000000000 --- a/app/javascript/src/handle-click.js +++ /dev/null @@ -1,16 +0,0 @@ -export function handleClick( - event, - nav, - removeNavExpandedClass, - addNavExpandedClass -) { - var isMobileNavExpanded = nav.popUp.hasClass(nav.expandedClass); - - event.preventDefault(); - - if (isMobileNavExpanded) { - removeNavExpandedClass(); - } else { - addNavExpandedClass(); - } -} diff --git a/app/javascript/src/mobile-nav.js b/app/javascript/src/mobile-nav.js deleted file mode 100644 index bb821db4643..00000000000 --- a/app/javascript/src/mobile-nav.js +++ /dev/null @@ -1,70 +0,0 @@ -import $ from "jquery"; -import { handleClick } from "src/handle-click"; - -$(function() { - // cache jQuery lookups into variables - // so we don't have to traverse the DOM every time - var sandwichIcon = $('.header__club-sandwich'); - var header = $('.header'); - var main = $('main'); - var footer = $('.footer'); - var signUpLink = $('.header__nav-link.js-sign-up-trigger'); - var navExpandedClass = 'mobile-nav-is-expanded'; - var headerSearch = $('.header__search'); - var headerLogo = $('.header__logo-wrap'); - - // variable to support mobile nav tab behaviour - // * skipSandwichIcon is for skipping sandwich icon - // when you tab from "gem" icon - // * tabDirection is for hiding and showing navbar - // when you tab in and out - var skipSandwichIcon = true; - var tabDirection = true; - - function removeNavExpandedClass() { - header.removeClass(navExpandedClass); - main.removeClass(navExpandedClass); - footer.removeClass(navExpandedClass); - } - - function addNavExpandedClass() { - header.addClass(navExpandedClass); - main.addClass(navExpandedClass); - footer.addClass(navExpandedClass); - } - - function handleFocusIn() { - if (skipSandwichIcon) { - addNavExpandedClass(); - headerSearch.focus(); - skipSandwichIcon = false; - } else { - removeNavExpandedClass(); - headerLogo.focus(); - skipSandwichIcon = true; - } - } - - sandwichIcon.click(function(e){ - var nav = {expandedClass: navExpandedClass, popUp: header} - handleClick(e, nav, removeNavExpandedClass, addNavExpandedClass); - }); - - sandwichIcon.on('focusin', handleFocusIn); - - signUpLink.on('focusin', function() { - if (!tabDirection) { - addNavExpandedClass(); - } - }); - - signUpLink.on('focusout', function() { - if (tabDirection) { - tabDirection = false; - removeNavExpandedClass(); - } else { - tabDirection = true; - addNavExpandedClass(); - } - }); -}); diff --git a/app/javascript/src/popup-nav.js b/app/javascript/src/popup-nav.js deleted file mode 100644 index ba76d451edf..00000000000 --- a/app/javascript/src/popup-nav.js +++ /dev/null @@ -1,22 +0,0 @@ -import $ from "jquery"; -import { handleClick } from "src/handle-click"; - -$(function() { - var arrowIcon = $('.header__popup-link'); - var popupNav = $('.header__popup__nav-links'); - - var navExpandedClass = 'is-expanded'; - - function removeNavExpandedClass() { - popupNav.removeClass(navExpandedClass); - } - - function addNavExpandedClass() { - popupNav.addClass(navExpandedClass); - } - - arrowIcon.click(function(e){ - var nav = {expandedClass: navExpandedClass, popUp: popupNav} - handleClick(e, nav, removeNavExpandedClass, addNavExpandedClass); - }); -}); diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index bc0206021fc..1dd042bb0a2 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -24,34 +24,36 @@ <%= javascript_importmap_tags %> - + <% if content_for?(:banner) %> <% end %> -
+
- <%= link_to(root_path, title: 'RubyGems', class: 'header__logo-wrap', data: { nav_target: "headerLogo" }) do %> + <%= link_to(root_path, title: 'RubyGems', class: 'header__logo-wrap', data: { nav_target: "logo" }) do %> RubyGems <% end %> - + Navigation menu
- <%= form_tag search_path, class: search_form_class, method: :get do %> - <%= search_field_tag :query, params[:query], placeholder: t(".header.search_gem_html"), class: "header__search", autocomplete: "off", data: { nav_target: "headerSearch" } %> -
    - <%= label_tag :query do %> - <%= t(".header.search_gem_html") %> + <% if request.path_info != "/" %> + <%= form_tag search_path, class: search_form_class, method: :get do %> + <%= search_field_tag :query, params[:query], placeholder: t(".header.search_gem_html"), class: "header__search", autocomplete: "off", data: { nav_target: "search" } %> +
      + <%= label_tag :query do %> + <%= t(".header.search_gem_html") %> + <% end %> + <%= submit_tag "⌕", id: "search_submit", name: nil, class: "header__search__icon" %> <% end %> - <%= submit_tag "⌕", id: "search_submit", name: nil, class: "header__search__icon" %> <% end %> -