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.

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

Stimulus for search field autocomplete #4468

Merged
merged 1 commit into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 28 additions & 8 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,6 @@ def stats_graph_meter(gem, count)
gem.downloads * 1.0 / count * 100
end

def search_form_class
if [root_path, advanced_search_path].include? request.path_info
"header__search-wrap--home"
else
"header__search-wrap"
end
end

def active?(path)
"is-active" if request.path_info == path
end
Expand All @@ -79,6 +71,34 @@ def flash_message(name, msg)
msg
end

def search_field(home: false)
data = {
autocomplete_target: "query",
action: %w[
autocomplete#suggest
keydown.down->autocomplete#next
keydown.up->autocomplete#prev
keydown.esc->autocomplete#hide
keydown.enter->autocomplete#clear
click@window->autocomplete#hide
focus->autocomplete#suggest
blur->autocomplete#hide
].join(" ")
}
data[:nav_target] = "search" unless home

search_field_tag(
:query,
params[:query],
placeholder: t("layouts.application.header.search_gem_html"),
autofocus: current_page?(root_url),
class: home ? "home__search" : "header__search",
autocomplete: "off",
aria: { autocomplete: "list" },
data: data
)
end

private

def default_avatar(theme:)
Expand Down
1 change: 0 additions & 1 deletion app/javascript/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import Rails from "@rails/ujs";
Rails.start();
import "controllers"

import "src/autocomplete";
import "src/clipboard_buttons";
import "src/multifactor_auths";
import "src/oidc_api_key_role_form";
Expand Down
103 changes: 103 additions & 0 deletions app/javascript/controllers/autocomplete_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import { Controller } from "@hotwired/stimulus"

// TODO: Add suggest help text and aria-live
Copy link
Member

Choose a reason for hiding this comment

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

Is this TODO for later (other PR)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's kind of a burden to add this here and requires some extra style and i18n work.

// https://accessibility.huit.harvard.edu/technique-aria-autocomplete
export default class extends Controller {
static targets = ["query", "suggestions", "template", "item"]
static classes = ["selected"]

connect() {
this.indexNumber = -1;
this.suggestLength = 0;
}

disconnect() { clear() }

clear() {
this.suggestionsTarget.innerHTML = ""
this.suggestionsTarget.removeAttribute('tabindex');
this.suggestionsTarget.removeAttribute('aria-activedescendant');
}

hide(e) {
// Allows adjusting the cursor in the input without hiding the suggestions.
if (!this.queryTarget.contains(e.target)) this.clear()
}

next() {
this.indexNumber++;
if (this.indexNumber >= this.suggestLength) this.indexNumber = 0;
this.focusItem(this.itemTargets[this.indexNumber]);
}

prev() {
martinemde marked this conversation as resolved.
Show resolved Hide resolved
this.indexNumber--;
if (this.indexNumber < 0) this.indexNumber = this.suggestLength - 1;
this.focusItem(this.itemTargets[this.indexNumber]);
}

// On mouseover, highlight the item, shifting the index,
// but don't change the input because it causes an undesireable feedback loop.
highlight(e) {
this.indexNumber = this.itemTargets.indexOf(e.currentTarget)
this.focusItem(e.currentTarget, false)
}

choose(e) {
martinemde marked this conversation as resolved.
Show resolved Hide resolved
this.clear();
this.queryTarget.value = e.target.textContent;
this.queryTarget.form.submit();
}

async suggest(e) {
const el = e.currentTarget;
const term = el.value.trim();

if (term.length >= 2) {
el.classList.remove('autocomplete-done');
el.classList.add('autocomplete-loading');
const query = new URLSearchParams({ query: term })

try {
const response = await fetch('/api/v1/search/autocomplete?' + query, { method: 'GET' })
const data = await response.json()
this.showSuggestions(data.slice(0, 10))
} catch (error) { }
el.classList.remove('autocomplete-loading');
el.classList.add('autocomplete-done');
} else {
this.clear()
}
}

showSuggestions(items) {
this.clear();
if (items.length === 0) {
return;
}
items.forEach((item, idx) => this.appendItem(item, idx));
this.suggestionsTarget.setAttribute('tabindex', 0);
this.suggestionsTarget.setAttribute('role', 'listbox');

this.suggestLength = items.length;
this.indexNumber = -1;
};

appendItem(text, idx) {
const clone = this.templateTarget.content.cloneNode(true);
const li = clone.querySelector('li')
li.textContent = text;
li.id = `suggest-${idx}`;
this.suggestionsTarget.appendChild(clone)
}

focusItem(el, change = true) {
this.itemTargets.forEach(el => el.classList.remove(this.selectedClass))
el.classList.add(this.selectedClass);
this.suggestionsTarget.setAttribute('aria-activedescendant', el.id);
if (change) {
this.queryTarget.value = el.textContent;
this.queryTarget.focus();
}
}
}
82 changes: 0 additions & 82 deletions app/javascript/src/autocomplete.js

This file was deleted.

2 changes: 1 addition & 1 deletion app/javascript/src/search.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import $ from "jquery";

if($("#advanced-search").length){
var $main = $('#home_query');
var $main = $('#query');
var $name = $('input#name');
var $summary = $('input#summary');
var $description = $('input#description');
Expand Down
14 changes: 1 addition & 13 deletions app/views/home/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,7 @@
<div class="home__image"></div>
</div>
<h1 class="home__heading"><%= t '.find_blurb' %></h1>
<div class="home__search-wrap">
<%= form_tag search_path, :method => :get do %>
<%= search_field_tag :query, params[:query], :placeholder => t('layouts.application.header.search_gem_html'), autofocus: current_page?(root_url), :id => 'home_query', :class => "home__search", :autocomplete => "off" %>
<ul id="suggest-home" class="suggest-list"></ul>
<%= label_tag :home_query do %>
<span class="t-hidden"><%= t('layouts.application.header.search_gem_html') %></span>
<center>
<%= link_to t("advanced_search"), advanced_search_path, class: "home__advanced__search t-link--has-arrow"%>
</center>
<% end %>
<%= submit_tag '⌕', :name => nil, :class => "home__search__icon" %>
<% end %>
</div>
<%= render "layouts/search" %>
<div class="home__cta-wrap">
<% if @downloads_count %>
<h2 class="home__downloads">
Expand Down
24 changes: 24 additions & 0 deletions app/views/layouts/_search.html.erb
Copy link
Member

Choose a reason for hiding this comment

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

should this be a partial instead of a layout? or a phlex component maybe?

Copy link
Member Author

@martinemde martinemde Feb 19, 2024

Choose a reason for hiding this comment

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

When you say "partial instead of layout" I'm confused. It's a partial in the layouts dir. should it move somewhere else?

Phlex components seem cool but they're also an extra barrier to entry. This is mostly code though, so it is reasonable. I thought about just making it a helper.

I'd be really happy to watch you run through the component stuff you made so I could start using it.

Copy link
Member

@simi simi Feb 19, 2024

Choose a reason for hiding this comment

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

Phlex components seem cool but they're also an extra barrier to entry. This is mostly code though, so it is reasonable. I thought about just making it a helper.

👍 I agree on this. Should we open discussion somewhere to get in sync on this topic? Meanwhile let's not block template changes because of this if possible.

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<% home = current_page?(root_path) || current_page?(advanced_search_path) %>
<div class="<%= home ? "home__search-wrap" : "header__search-wrap" %>" role="search">
<%= form_tag search_path, method: :get, data: { controller: "autocomplete", autocomplete_selected_class: "selected", } do %>
<%= search_field(home: home) %>

<ul class="suggest-list" role="listbox" data-autocomplete-target="suggestions"></ul>

<template id="suggestion" data-autocomplete-target="template">
<li class="menu-item" role="option" tabindex="-1" data-autocomplete-target="item" data-action="click->autocomplete#choose mouseover->autocomplete#highlight"></li>
</template>

<%= label_tag :query, id: "querylabel" do %>
<span class="t-hidden"><%= t('layouts.application.header.search_gem_html') %></span>
<% end %>

<%= submit_tag '⌕', id: "search_submit", name: nil, class: home ? "home__search__icon" : "header__search__icon", aria: { labelledby: "querylabel" } %>

<% if home %>
<center>
<%= link_to t("advanced_search"), advanced_search_path, class: "home__advanced__search t-link--has-arrow"%>
</center>
<% end %>
<% end %>
</div>
11 changes: 1 addition & 10 deletions app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,7 @@
</a>

<div class="header__nav-links-wrap">
<% 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 %>
<% end %>
<%= render "layouts/search" unless current_page?(root_path) || current_page?(advanced_search_path) %>

<nav class="header__nav-links" data-controller="dropdown">
<% if signed_in? %>
Expand Down
6 changes: 3 additions & 3 deletions app/views/searches/advanced.html.erb
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
<% @title = t("advanced_search") %>

<div class="home__search-wrap">
<div class="home__search-wrap" role="search">
<%= form_tag search_url, id: "advanced-search", method: :get do %>
<%= search_field_tag :query, params[:query], placeholder: t("layouts.application.header.search_gem_html"), autofocus: current_page?(root_url), id: "home_query", class: "home__search" %>
<%= label_tag :home_query do %>
<%= search_field_tag :query, params[:query], placeholder: t("layouts.application.header.search_gem_html"), autofocus: current_page?(root_url), id: "query", class: "home__search" %>
<%= label_tag :query do %>
<span class="t-hidden"><%= t("layouts.application.header.search_gem_html") %></span>
<% end %>
<%= submit_tag "⌕", id: "advanced_search_submit", name: nil, class: "home__search__icon" %>
Expand Down
2 changes: 1 addition & 1 deletion test/system/advanced_search_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class AdvancedSearchTest < ApplicationSystemTestCase

import_and_refresh

fill_in "home_query", with: "downloads: <5"
fill_in "query", with: "downloads: <5"
click_button "advanced_search_submit"

assert_current_path(search_path, ignore_query: true)
Expand Down
10 changes: 5 additions & 5 deletions test/system/autocompletes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class AutocompletesTest < ApplicationSystemTestCase
import_and_refresh

visit root_path
@fill_field = find_by_id "home_query"
@fill_field = find_by_id "query"
@fill_field.set "rubo"
page.find(".autocomplete-done", wait: Capybara.default_max_wait_time)
end
Expand Down Expand Up @@ -53,25 +53,25 @@ class AutocompletesTest < ApplicationSystemTestCase
test "down arrow key to choose suggestion" do
@fill_field.native.send_keys :down

assert page.has_no_field? "home_query", with: "rubo"
assert page.has_no_field? "query", with: "rubo"
end

test "up arrow key to choose suggestion" do
@fill_field.native.send_keys :up

assert page.has_no_field? "home_query", with: "rubo"
assert page.has_no_field? "query", with: "rubo"
end

test "down arrow key should loop" do
@fill_field.native.send_keys :down, :down, :down, :down

assert find_by_id("suggest-home").all(".menu-item").last.matches_css?(".selected")
assert find(".suggest-list").all(".menu-item").last.matches_css?(".selected")
end

test "up arrow key should loop" do
@fill_field.native.send_keys :up, :up, :up, :up

assert find_by_id("suggest-home").all(".menu-item").first.matches_css?(".selected")
assert find(".suggest-list").all(".menu-item").first.matches_css?(".selected")
end

test "mouse hover a suggest item to choose suggestion" do
Expand Down
Loading