maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix duplicate IDs on homepage (#17092)

Browse source 
* fix duplicate ID main nav

* fix duplicate ID other nav headings

* fix duplicate ID reading list count

* fix duplicate attribute class

* missing case readingCountList on feed

* missing dot in query selector

* Revert "fix duplicate ID other nav headings"

This reverts commit 4f495962c47e08f0f6c41e33e897b2db176432f5.

* use generic class 'other-nav' for tests

* pass a variable 'context' to the partial nav

* pass var 'context' to partial

* fix unwanted typo in partial sidebar

* Update app/views/admin/navigation_links/index.html.erb

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* Update app/assets/javascripts/initializers/initializeReadingListIcons.js

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* Update app/assets/javascripts/initializers/initializeReadingListIcons.js

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* js-reading-list-count renaming

* use data-testid instead of generic classes for tests

* fix typo in specs

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
This commit is contained in:
Julien Maury 2022-04-14 02:48:55 -07:00 • committed by GitHub
parent 3a753cd7f6
commit 01c1d44e65
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 46 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

44
app/assets/javascripts/initializers/initializeReadingListIcons.js
View file

@ -33,14 +33,17 @@ function highlightButton(button) {
}
function addReadingListCountToHomePage() {
var user = userData();
var readingListCount;
if (user && document.getElementById('reading-list-count')) {
readingListCount =
user.reading_list_ids.length > 0 ? user.reading_list_ids.length : '';
document.getElementById('reading-list-count').innerHTML = readingListCount;
document.getElementById('reading-list-count').dataset.count =
user.reading_list_ids.length;
const user = userData();
const readingListContainers = document.querySelectorAll(
'.js-reading-list-count',
);
if (user && readingListContainers) {
readingListContainers.forEach(function (e) {
const readingListCount =
user.reading_list_ids.length > 0 ? user.reading_list_ids.length : '';
e.innerHTML = readingListCount;
e.dataset.count = user.reading_list_ids.length;
});
}
}
@ -83,17 +86,22 @@ function renderButtonState(button, json) {
}
function renderNewSidebarCount(button, json) {
var newCount;
var count = document.getElementById('reading-list-count').dataset.count;
count = parseInt(count, 10);
if (json.result === 'create') {
newCount = count + 1;
} else if (count !== 0) {
newCount = count - 1;
const readingListContainers = document.querySelectorAll(
'.js-reading-list-count',
);
if (readingListContainers) {
readingListContainers.forEach(function (e) {
const count = parseInt(e.dataset.count, 10);
let newCount;
if (json.result === 'create') {
newCount = count + 1;
} else if (count !== 0) {
newCount = count - 1;
}
e.dataset.count = newCount;
e.innerHTML = newCount > 0 ? newCount : '';
});
}
document.getElementById('reading-list-count').dataset.count = newCount;
document.getElementById('reading-list-count').innerHTML =
newCount > 0 ? newCount : '';
}
function buttonFormData(button) {

4
app/views/admin/navigation_links/index.html.erb
View file

@ -17,7 +17,7 @@
<div class="crayons-icon crayons-icon--default">
<%= link[:icon].html_safe %>
</div>
<%= link[:name] %><span id="reading-list-count" class="c-indicator"></span>
<%= link[:name] %><span class="js-reading-list-count c-indicator"></span>
</a>
</div>
@ -45,7 +45,7 @@
<div class="crayons-icon crayons-icon--default">
<%= link[:icon].html_safe %>
</div>
<%= link[:name] %><span id="reading-list-count" class="c-indicator"></span>
<%= link[:name] %><span class="js-reading-list-count c-indicator"></span>
</a>
</div>

2
app/views/articles/_sidebar.html.erb
View file

@ -2,7 +2,7 @@
<aside class="side-bar" aria-label="Primary sidebar">
<% cache(release_adjusted_cache_key("main-sidebar-nav--#{user_signed_in?}"), expires_in: 15.minutes) do %>
<%= render "shared/auth_widget" unless user_signed_in? %>
<%= render "layouts/main_nav" %>
<%= render partial: "layouts/main_nav", locals: { context: "sidebar" } %>
<%= render "layouts/sidebar_tags" %>
<% @sponsorships = Sponsorship.gold.live.includes(:organization).order(featured_number: :asc) %>
<%# the pattern .present?/.each has the advantage of issuing only a single SQL query to load objects in memory %>

6
app/views/layouts/_main_nav.html.erb
View file

@ -1,7 +1,7 @@
<% default_nav_links = NavigationLink.default_section.ordered.to_a %>
<% other_nav_links = NavigationLink.other_section.ordered.to_a %>
<nav class="mb-4 <% unless user_signed_in? %>mt-4<% end %>" id="main-navigation" aria-label="<%= community_name %>">
<nav class="mb-4 <% unless user_signed_in? %>mt-4<% end %>" data-testid="main-nav" aria-label="<%= community_name %>">
<ul class="default-navigation-links sidebar-navigation-links spec-sidebar-navigation-links">
<% default_nav_links.each do |link| %>
<%= render "layouts/sidebar_nav_link", link: link %>
@ -13,8 +13,8 @@
visible based on the current state (see
ApplicationHelper#display_navigation_link? for details). %>
<% if other_nav_links.any? %>
<nav class="mb-4" aria-labelledby="other-nav-heading">
<h2 id="other-nav-heading" class="crayons-subtitle-3 py-2 pl-3">
<nav class="mb-4" data-testid="other-nav" aria-labelledby="other-nav-heading-<%= context %>">
<h2 id="other-nav-heading-<%= context %>" class="crayons-subtitle-3 py-2 pl-3">
<%= t("views.main.nav.other") %>
</h2>
<ul class="other-navigation-links sidebar-navigation-links spec-sidebar-navigation-links">

2
app/views/layouts/_sidebar_nav_link.html.erb
View file

@ -6,7 +6,7 @@
</span>
<%= t("views.main.nav_name.#{link.name}", default: link.name) %>
<% if link.url.include?("readinglist") %>
<span id="reading-list-count" class="c-indicator ml-2 self-center"></span>
<span class="js-reading-list-count c-indicator ml-2 self-center"></span>
<% end %>
</a>
</li>

2
app/views/shared/_hamburger.html.erb
View file

@ -10,7 +10,7 @@
<div class="p-2">
<%= render partial: "shared/auth_widget" unless user_signed_in? %>
<%= render partial: "layouts/main_nav" %>
<%= render partial: "layouts/main_nav", locals: { context: "hamburger" } %>
</div>
</div>
<div class="hamburger__overlay js-hamburger-trigger"></div>

24
spec/system/homepage/user_visits_homepage_spec.rb
View file

@ -62,28 +62,28 @@ RSpec.describe "User visits a homepage", type: :system do
end
it "shows expected number of links when signed out" do
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_selector(".sidebar-navigation-link", count: 1)
end
within("nav[aria-labelledby='other-nav-heading']", match: :first) do
within("nav[data-testid='other-nav']", match: :first) do
expect(page).to have_selector(".sidebar-navigation-link", count: 2)
end
end
it "shows the Other section when other nav links exist" do
within("nav[aria-labelledby='other-nav-heading']", match: :first) do
within("nav[data-testid='other-nav']", match: :first) do
expect(page).to have_selector(".other-navigation-links")
end
NavigationLink.other_section.destroy_all
visit "/"
expect(page).not_to have_selector("nav[aria-labelledby='other-nav-heading']")
expect(page).not_to have_selector("nav[data-testid='other-nav']")
end
it "hides link when display_only_when_signed_in is true" do
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_selector(".default-navigation-links .sidebar-navigation-link", count: 1)
end
end
@ -96,7 +96,7 @@ RSpec.describe "User visits a homepage", type: :system do
position: 3)
visit "/"
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_selector(".default-navigation-links li:nth-child(1)", text: "Shop")
expect(page).to have_selector(".default-navigation-links li:nth-child(2)", text: "Mock")
end
@ -191,35 +191,35 @@ RSpec.describe "User visits a homepage", type: :system do
end
it "shows the correct navigation_links" do
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_text(navigation_link_1.name)
expect(page).to have_text(navigation_link_3.name)
end
within("nav[aria-labelledby='other-nav-heading']", match: :first) do
within("nav[data-testid='other-nav']", match: :first) do
expect(page).to have_text(navigation_link_2.name)
end
end
it "shows the correct urls" do
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_link(href: navigation_link_1.url)
expect(page).to have_link(href: navigation_link_3.url)
end
within("nav[aria-labelledby='other-nav-heading']", match: :first) do
within("nav[data-testid='other-nav']", match: :first) do
expect(page).to have_link(href: navigation_link_2.url)
end
end
it "shows expected # of links when signed in" do
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_selector(".sidebar-navigation-link", count: 2) # it's count: 1 when signed out
end
end
it "shows link when display_only_when_signed_in is true" do
within("#main-navigation", match: :first) do
within("nav[data-testid='main-nav']", match: :first) do
expect(page).to have_selector(".default-navigation-links li:nth-child(2)", text: "Beauty")
end
end