Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor stimulus nav controllers to follow stimulus best practices #4422

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Refactor stimulus nav controllers to follow stimulus best practices #4422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions app/assets/stylesheets/modules/header.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,25 +185,25 @@
border-radius: 22px; }

@media (min-width: 1020px) {
.header__popup__nav-links, .home__header__popup__nav-links {
Copy link
Member Author

@martinemde martinemde Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSS changes are flipping the logic from is-expanded to hidden to be in line with the stimulus-dropdown component (which I'm not using, but still conformed to for simplicity).

.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;
Expand All @@ -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;
Expand Down
18 changes: 18 additions & 0 deletions app/javascript/controllers/dropdown_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');
}
}
96 changes: 29 additions & 67 deletions app/javascript/controllers/nav_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) }
}
16 changes: 0 additions & 16 deletions app/javascript/src/handle-click.js
View file
Open in desktop

This file was deleted.

70 changes: 0 additions & 70 deletions app/javascript/src/mobile-nav.js
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions app/javascript/src/popup-nav.js
View file
Open in desktop

This file was deleted.

32 changes: 17 additions & 15 deletions app/views/layouts/application.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,34 +24,36 @@
<%= javascript_importmap_tags %>
</head>

<body class="<%= 'body--index' if request.path_info == '/' %>" data-controller="nav">
<body class="<%= 'body--index' if request.path_info == '/' %>" data-controller="nav" data-nav-expanded-class="mobile-nav-is-expanded">
<!-- Top banner -->
<% if content_for?(:banner) %>
<div class="banner">
<%= yield :banner %>
</div>
<% end %>
<header class="header <%= 'header--interior' if request.path_info != '/' %>" data-nav-target="header">
<header class="header <%= 'header--interior' if request.path_info != '/' %>" data-nav-target="header collapse">
<div class="l-wrap--header">
<%= 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 %>
<span class="header__logo" data-icon="⬡">⬢</span>
<span class="t-hidden">RubyGems</span>
<% end %>
<a class="header__club-sandwich" href="#" data-nav-target="sandwichIcon">
<a class="header__club-sandwich" href="#" data-action="nav#toggle focusin->nav#focus click@window->nav#hide">
<span class="t-hidden">Navigation menu</span>
</a>

<div class="header__nav-links-wrap">
<%= 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" } %>
<ul id="suggest" class="suggest-list"></ul>
<%= label_tag :query do %>
<span class="t-hidden"><%= t(".header.search_gem_html") %></span>
<% 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" } %>
<ul id="suggest" class="suggest-list"></ul>
<%= label_tag :query do %>
<span class="t-hidden"><%= t(".header.search_gem_html") %></span>
<% 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 %>

<nav class="header__nav-links">
<nav class="header__nav-links" data-controller="dropdown">
<% if signed_in? %>
<a href="<%= profile_path(current_user.display_id) %>" class="header__nav-link mobile__header__nav-link">
<span><%= current_user.name %></span>
Expand All @@ -75,10 +77,10 @@
<span><%= current_user.name %></span>
<%= avatar 80, "user_gravatar", theme: :dark %>
</a>
<a href="#" class="header__popup-link" data-icon="▼" data-nav-target="dropdownButton">
<a href="#" class="header__popup-link" data-icon="▼" data-action="dropdown#toggle click@window->dropdown#hide">
<span class="t-hidden">More items</span>
</a>
<div class="header__popup__nav-links" data-nav-target="dropdown">
<div class="header__popup__nav-links hidden" data-dropdown-target="menu" >
<%= link_to t('.header.settings'), edit_settings_path, class: "header__nav-link" %>
<%= link_to t('.header.dashboard'), dashboard_path, class: "header__nav-link" %>
<%= link_to t('.header.edit_profile'), edit_profile_path, class: "header__nav-link" %>
Expand Down Expand Up @@ -109,7 +111,7 @@
</div>
<% end %>

<main class="<%= 'main--interior' if request.path_info != '/' %>" data-nav-target="main">
<main class="<%= 'main--interior' if request.path_info != '/' %>" data-nav-target="collapse">
<% if request.path_info != '/' %>
<div class="l-wrap--b">
<% if @title %>
Expand All @@ -129,7 +131,7 @@
<% end %>
</main>

<footer class="footer" data-nav-target="footer">
<footer class="footer" data-nav-target="collapse">
<div class="l-wrap--footer">
<div class="l-overflow">
<div class="nav--v l-col--r--pad">
Expand Down
Loading