From d3c4f270e03d01ec4343597b64b516c927dcdf25 Mon Sep 17 00:00:00 2001 From: Ian MacFarland Date: Fri, 26 Mar 2021 18:41:18 -0700 Subject: [PATCH] remove direct section linking --- spotlight-client/src/App.tsx | 4 +- .../src/NarrativeLayout/NarrativeLayout.tsx | 34 +----- .../NarrativeNavigation/AdvanceLink.tsx | 19 ++-- .../NarrativeNavigation.tsx | 28 ++--- .../NarrativeNavigation/SectionLinks.tsx | 16 +-- .../NarrativeNavigation/types.ts | 18 ++++ .../SystemNarrativePage.test.tsx | 102 +----------------- 7 files changed, 49 insertions(+), 172 deletions(-) create mode 100644 spotlight-client/src/NarrativeLayout/NarrativeNavigation/types.ts diff --git a/spotlight-client/src/App.tsx b/spotlight-client/src/App.tsx index d9240139..de6db9e4 100644 --- a/spotlight-client/src/App.tsx +++ b/spotlight-client/src/App.tsx @@ -78,9 +78,7 @@ const App: React.FC = () => { - + diff --git a/spotlight-client/src/NarrativeLayout/NarrativeLayout.tsx b/spotlight-client/src/NarrativeLayout/NarrativeLayout.tsx index 76a2ca1f..aae83dc6 100644 --- a/spotlight-client/src/NarrativeLayout/NarrativeLayout.tsx +++ b/spotlight-client/src/NarrativeLayout/NarrativeLayout.tsx @@ -15,7 +15,6 @@ // along with this program. If not, see . // ============================================================================= -import { navigate, useParams } from "@reach/router"; import useBreakpoint from "@w11r/use-breakpoint"; import { rem } from "polished"; import React, { useCallback, useEffect, useRef, useState } from "react"; @@ -24,8 +23,6 @@ import { useSpring } from "react-spring/web.cjs"; import Sticker from "react-stickyfill"; import styled from "styled-components/macro"; import { NAV_BAR_HEIGHT } from "../constants"; -import getUrlForResource from "../routerUtils/getUrlForResource"; -import normalizeRouteParams from "../routerUtils/normalizeRouteParams"; import { X_PADDING } from "../SystemNarrativePage/constants"; import NarrativeNavigation from "./NarrativeNavigation"; import { LayoutSection } from "./types"; @@ -58,7 +55,6 @@ type NarrativeLayoutProps = { }; const NarrativeLayout: React.FC = ({ sections }) => { - const routeParams = useParams(); const sectionsContainerRef = useRef() as React.MutableRefObject< HTMLDivElement >; @@ -73,10 +69,7 @@ const NarrativeLayout: React.FC = ({ sections }) => { isScrolling = false; }; - const [activeSection, directlySetActiveSection] = useState( - // make sure we consume the section number in the URL, if any, on first mount - Number(routeParams.sectionNumber) || 1 - ); + const [activeSection, directlySetActiveSection] = useState(1); // wrap the section state setter in a function that respects the flag const setActiveSection = useCallback( (sectionNumber: number) => { @@ -86,10 +79,6 @@ const NarrativeLayout: React.FC = ({ sections }) => { }, [isScrolling] ); - // keep section state in sync with URL if it changes externally (e.g. via nav link) - useEffect(() => { - directlySetActiveSection(Number(routeParams.sectionNumber) || 1); - }, [routeParams.sectionNumber]); const [, setScrollSpring] = useSpring(() => ({ onFrame: (props: { top: number }) => { @@ -110,7 +99,6 @@ const NarrativeLayout: React.FC = ({ sections }) => { to: { top: window.pageYOffset }, })); - const { tenantId, narrativeTypeId } = normalizeRouteParams(routeParams); // updating the active section has two key side effects: // 1. smoothly scrolling to the active section // 2. updating the page URL so the section can be linked to directly @@ -132,25 +120,8 @@ const NarrativeLayout: React.FC = ({ sections }) => { from: { top: window.pageYOffset }, reset: true, }); - - // these should always be defined on this page; more type safety - if (tenantId && narrativeTypeId) { - navigate( - `${getUrlForResource({ - page: "narrative", - params: { tenantId, narrativeTypeId }, - })}/${activeSection}`, - { replace: true } - ); - } } - }, [ - activeSection, - narrativeTypeId, - sectionsContainerRef, - setScrollSpring, - tenantId, - ]); + }, [activeSection, sectionsContainerRef, setScrollSpring]); return ( @@ -161,6 +132,7 @@ const NarrativeLayout: React.FC = ({ sections }) => { diff --git a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/AdvanceLink.tsx b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/AdvanceLink.tsx index 554ef76c..5304e5a3 100644 --- a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/AdvanceLink.tsx +++ b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/AdvanceLink.tsx @@ -18,27 +18,29 @@ import { rem } from "polished"; import React, { useState } from "react"; import styled from "styled-components/macro"; -import NavigationLink from "../../NavigationLink"; import { colors, Chevron } from "../../UiLibrary"; -const StyledNavLink = styled(NavigationLink)` +const Button = styled.button.attrs({ type: "button" })` + background: none; + border: none; + cursor: ${(props) => (props.disabled ? "not-allowed" : "pointer")}; padding: ${rem(8)}; `; type AdvanceLinkProps = { activeSection: number; disabled: boolean; + setActiveSection: (section: number) => void; type: "previous" | "next"; - urlBase: string; }; const AdvanceLink: React.FC = ({ activeSection, disabled, + setActiveSection, type, - urlBase, }) => { - let targetSection; + let targetSection: number; let direction: "up" | "down"; if (type === "previous") { @@ -54,10 +56,9 @@ const AdvanceLink: React.FC = ({ const color = hovered && !disabled ? colors.accent : undefined; return ( - !disabled && setActiveSection(targetSection)} disabled={disabled} - replace aria-label={`${type} section`} onMouseOver={() => setHovered(true)} onFocus={() => setHovered(true)} @@ -65,7 +66,7 @@ const AdvanceLink: React.FC = ({ onBlur={() => setHovered(false)} > - + ); }; diff --git a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/NarrativeNavigation.tsx b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/NarrativeNavigation.tsx index 5ac9da09..44a4023f 100644 --- a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/NarrativeNavigation.tsx +++ b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/NarrativeNavigation.tsx @@ -15,17 +15,14 @@ // along with this program. If not, see . // ============================================================================= -import { useParams } from "@reach/router"; import { format } from "d3-format"; import { rem } from "polished"; import React from "react"; import styled from "styled-components/macro"; -import { DeepNonNullable } from "utility-types"; -import getUrlForResource from "../../routerUtils/getUrlForResource"; -import normalizeRouteParams from "../../routerUtils/normalizeRouteParams"; import { LayoutSection } from "../types"; import AdvanceLink from "./AdvanceLink"; import SectionLinks from "./SectionLinks"; +import { SetSection } from "./types"; const formatPageNum = format("02"); @@ -52,27 +49,14 @@ const SectionNumberFaded = styled(SectionNumber)` type NavigationProps = { activeSection: number; sections: LayoutSection[]; + setActiveSection: SetSection; }; const SectionNavigation: React.FC = ({ activeSection, sections, + setActiveSection, }) => { - const { tenantId, narrativeTypeId } = normalizeRouteParams( - useParams() - // these keys should always be present on this page - ) as DeepNonNullable< - Pick< - ReturnType, - "tenantId" | "narrativeTypeId" - > - >; - // base is the current page, minus the section number - const urlBase = getUrlForResource({ - page: "narrative", - params: { tenantId, narrativeTypeId }, - }); - const totalPages = sections.length; // these will be used to toggle prev/next links @@ -83,17 +67,17 @@ const SectionNavigation: React.FC = ({ {formatPageNum(activeSection)} {formatPageNum(totalPages)} - + diff --git a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/SectionLinks.tsx b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/SectionLinks.tsx index f621eb34..1cd36dac 100644 --- a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/SectionLinks.tsx +++ b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/SectionLinks.tsx @@ -15,13 +15,13 @@ // along with this program. If not, see . // ============================================================================= -import { Link } from "@reach/router"; import { rem } from "polished"; import React, { useEffect } from "react"; import { animated, useSpring, useSprings } from "react-spring/web.cjs"; import styled from "styled-components/macro"; import { colors } from "../../UiLibrary"; import { LayoutSection } from "../types"; +import { SetSection } from "./types"; import { THUMB_SIZE } from "./utils"; const PageProgressContainer = styled.div` @@ -64,13 +64,17 @@ const SectionList = styled.ul` const SectionListItem = styled.li``; -const SectionLink = styled(Link)` +const SectionLink = styled.button.attrs({ type: "button" })` + border: none; + background: none; color: ${colors.text}; + cursor: pointer; display: flex; height: ${rem(THUMB_SIZE.height)}; justify-content: center; margin-bottom: ${rem(THUMB_SIZE.paddingBottom)}; position: relative; + text-align: left; width: 100%; `; @@ -100,8 +104,8 @@ const getThumbOffset = (activeSection: number) => const SectionLinks: React.FC<{ activeSection: number; sections: LayoutSection[]; - urlBase: string; -}> = ({ activeSection, sections, urlBase }) => { + setActiveSection: SetSection; +}> = ({ activeSection, sections, setActiveSection }) => { const totalPages = sections.length; const progressBarHeight = @@ -169,8 +173,8 @@ const SectionLinks: React.FC<{ return ( setActiveSection(index + 1)} onMouseOver={showLinkLabel(index)} onFocus={showLinkLabel(index)} onMouseOut={hideLinkLabel(index)} diff --git a/spotlight-client/src/NarrativeLayout/NarrativeNavigation/types.ts b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/types.ts new file mode 100644 index 00000000..35f7d9d8 --- /dev/null +++ b/spotlight-client/src/NarrativeLayout/NarrativeNavigation/types.ts @@ -0,0 +1,18 @@ +// Recidiviz - a data platform for criminal justice reform +// Copyright (C) 2021 Recidiviz, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . +// ============================================================================= + +export type SetSection = (section: number) => void; diff --git a/spotlight-client/src/SystemNarrativePage/SystemNarrativePage.test.tsx b/spotlight-client/src/SystemNarrativePage/SystemNarrativePage.test.tsx index ae15c633..dac7680c 100644 --- a/spotlight-client/src/SystemNarrativePage/SystemNarrativePage.test.tsx +++ b/spotlight-client/src/SystemNarrativePage/SystemNarrativePage.test.tsx @@ -15,7 +15,7 @@ // along with this program. If not, see . // ============================================================================= -import { fireEvent, screen, waitFor, within } from "@testing-library/react"; +import { screen, within } from "@testing-library/react"; import mockContentFixture from "./__fixtures__/contentSource"; import { SystemNarrativeContent } from "../contentApi/types"; import { renderNavigableApp } from "../testUtils"; @@ -42,106 +42,6 @@ test("renders all the sections", () => { }); }); -test("navigate to previous and next section", async () => { - const { history } = renderNavigableApp({ - route: `/us-nd/${NarrativesSlug}/parole`, - }); - - const nextLabel = "next section"; - const prevLabel = "previous section"; - - const navRegion = screen.getByRole("navigation", { name: "page sections" }); - - const nextLink = screen.getByRole("link", { name: nextLabel }); - - // no previous on the first section - expect(screen.queryByRole("link", { name: prevLabel })).toHaveAttribute( - "aria-disabled", - "true" - ); - expect(nextLink).toBeInTheDocument(); - - expect(within(navRegion).getByText("01")).toBeInTheDocument(); - - // advance to section 2 - fireEvent.click(nextLink); - - await waitFor(() => { - expect(within(navRegion).getByText("02")).toBeInTheDocument(); - }); - expect(history.location.pathname).toBe(`/us-nd/${NarrativesSlug}/parole/2`); - - const prevLink = screen.getByRole("link", { name: prevLabel }); - expect(prevLink).toBeInTheDocument(); - - // return to section 1 again - fireEvent.click(prevLink); - await waitFor(() => - expect(within(navRegion).getByText("01")).toBeInTheDocument() - ); - expect(screen.queryByRole("link", { name: prevLabel })).toHaveAttribute( - "aria-disabled", - "true" - ); - - // advance to the last section - history.navigate( - `/us-nd/${NarrativesSlug}/parole/${narrativeContent.sections.length + 1}` - ); - - await waitFor(() => { - // both of the page numbers should be the same, e.g. 08/08 - expect( - within(navRegion).getAllByText(`0${narrativeContent.sections.length + 1}`) - .length - ).toBe(2); - }); - - // no next on the last section - expect( - // the element referenced by nextLink is still present, - // but it no longer has role=link - screen.queryByRole("link", { name: nextLabel }) - ).toHaveAttribute("aria-disabled", "true"); - - // JSDOM don't support layout features - // so we can't really test anything related to scroll position :( -}); - -test("navigate directly to any section", async () => { - const { history } = renderNavigableApp({ - route: `/us-nd/${NarrativesSlug}/parole`, - }); - - const navRegion = screen.getByRole("navigation", { name: "page sections" }); - - await Promise.all( - narrativeContent.sections.map(async (section, index) => { - const linkToSection = within(navRegion).getByRole("link", { - name: section.title, - }); - - fireEvent.click(linkToSection); - - await waitFor(() => - expect(history.location.pathname).toBe( - `/us-nd/${NarrativesSlug}/parole/${index + 2}` - ) - ); - }) - ); - - const introLink = within(navRegion).getByRole("link", { - name: narrativeContent.title, - }); - - fireEvent.click(introLink); - - await waitFor(() => - expect(history.location.pathname).toBe(`/us-nd/${NarrativesSlug}/parole/1`) - ); -}); - test("renders link tags in copy", async () => { // this fixture has links in the copy renderNavigableApp({ route: `/us-nd/${NarrativesSlug}/sentencing` });