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

Feat: Add Mobile styles to FooterNav #181

Merged
merged 9 commits into from
May 22, 2020
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/* eslint-disable jsx-a11y/anchor-is-valid, react/jsx-key */
import React from 'react'

import { FooterExtendedNavList } from './FooterExtendedNavList'

export default {
title: 'Footer/FooterExtendedNavList',
parameters: {
info: `
Display grouped nav items in an extended nav. Used in "big" USWDS 2.0 Footer component.

Source: https://designsystem.digital.gov/components/form-controls/#footer
`,
},
}

export const Default = (): React.ReactElement => (
<div className="usa-footer--big ">
haworku marked this conversation as resolved.
Show resolved Hide resolved
<FooterExtendedNavList
nestedLinks={[
['Topic', ...Array(3).fill(<a href="#">Secondary link</a>)],
[
'Topic',
<a key="2" href="#">
Secondary link that is pretty long
</a>,
...Array(2).fill(<a href="#">Secondary link</a>),
],
['Topic', ...Array(3).fill(<a href="#">Secondary link</a>)],
]}
/>
</div>
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/* eslint-disable jsx-a11y/anchor-is-valid, react/jsx-key */

import React from 'react'
import { act, fireEvent, render } from '@testing-library/react'

import { FooterExtendedNavList } from './FooterExtendedNavList'

const links = [
[
'Types of Cats',
...Array(2).fill(
<a className="usa-footer__secondary-link" href="#">
Cheetah
</a>
),
],
[
'Musical Gifts',
...Array(3).fill(
<a className="usa-footer__secondary-link" href="#">
Purple Rain
</a>
),
],
]

describe('FooterExtendedNavList component', () => {
it('renders without errors', () => {
const { container } = render(<FooterExtendedNavList nestedLinks={links} />)
expect(container.querySelector('ul')).toBeInTheDocument()
})

it('renders headings', () => {
const { container, getByText } = render(
<FooterExtendedNavList nestedLinks={links} />
)
expect(container.querySelectorAll('h4')).toHaveLength(2)
expect(getByText('Types of Cats')).toBeInTheDocument()
expect(getByText('Musical Gifts')).toBeInTheDocument()
})

it('renders links', () => {
const { container, getAllByText } = render(
<FooterExtendedNavList nestedLinks={links} />
)
expect(container.querySelectorAll('a')).toHaveLength(5)
expect(getAllByText('Purple Rain')).toHaveLength(3)
expect(getAllByText('Cheetah')).toHaveLength(2)
})

describe('isMobile', () => {
it.todo(
'renders mobile view when client window width is less than threshold'
Copy link
Contributor Author

@haworku haworku May 18, 2020

Choose a reason for hiding this comment

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

I can't figure out how to test window resize with running into typescript read-only limitations on reassigning window objects during a test. Ideally, I was going to use jest beforeAll to adjust the window size to mobile before running all the tests in this describe block.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you use the resizeTo method instead of reassigning the dimensions? https://developer.mozilla.org/en-US/docs/Web/API/Window/resizeTo

imo this would be a great test to have!

Copy link
Contributor Author

@haworku haworku May 21, 2020

Choose a reason for hiding this comment

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

Tried out resizeTo but its not implemented. jest incorporates JSDOM to override window - and JSDOM doesn't support all the same methods. Ended up overriding tyepscript and then eslint warnings about overriding typescript 😆 I hadn't realized I could just do that and move on.

)

it('renders headings', () => {
const { container, getByText } = render(
<FooterExtendedNavList nestedLinks={links} />
)
expect(container.querySelectorAll('h4')).toHaveLength(2)
expect(getByText('Types of Cats')).toBeInTheDocument()
expect(getByText('Musical Gifts')).toBeInTheDocument()
})

it('renders link sections as collapsed on initial load', () => {
const { getAllByText } = render(
<FooterExtendedNavList nestedLinks={links} />
)
expect(getAllByText('Cheetah')).not.toBeInTheDocument
expect(getAllByText('Purple Rain')).not.toBeInTheDocument
})

it('toggles section visibility onClick', () => {
const { getByText, getAllByText } = render(
<FooterExtendedNavList nestedLinks={links} />
)

fireEvent.click(getByText('Types of Cats'))

expect(getAllByText('Cheetah')).toHaveLength(2)
expect(getAllByText('Purple Rain')).not.toBeInTheDocument
})

it('only toggles one section expanded at a time', () => {
const { getAllByText, getByText } = render(
<FooterExtendedNavList nestedLinks={links} />
)

fireEvent.click(getByText('Types of Cats'))
fireEvent.click(getByText('Musical Gifts'))

expect(getAllByText('Purple Rain')).toBeInTheDocument
expect(getAllByText('Cheetah')).not.toBeInTheDocument
})
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import React, { useState, useEffect } from 'react'
import classnames from 'classnames'
import { NavList } from '../../header/NavList/NavList'

export type ExtendedNavLinksType = React.ReactNode[][]

type FooterExtendedNavListProps = {
/*
Multidimensional array of grouped nav links. Sub-arrays are column sections, first element is used as a heading.
*/
nestedLinks: ExtendedNavLinksType
haworku marked this conversation as resolved.
Show resolved Hide resolved
}

export const FooterExtendedNavList = ({
className,
nestedLinks,
}: FooterExtendedNavListProps &
React.HTMLAttributes<HTMLElement>): React.ReactElement => {
const classes = classnames('grid-row grid-gap-4', className)

const [isMobile, setIsMobile] = React.useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is super minor but I noticed there is a brief flash of the desktop nav at mobile sizes on render. I think that could be resolved by doing the initial window size check here to default the state to the correct value (instead of false) and then that would remove the need to trigger handleResize() in the effect hook

const [sectionsOpenState, setSectionsOpenState] = useState<boolean[]>(
Array(nestedLinks.length).fill(false)
)

useEffect(() => {
const isClient = window && typeof window === 'object'
haworku marked this conversation as resolved.
Show resolved Hide resolved
if (!isClient) return
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: just for performance, couldn't we also do an early return if isMobile is defined? since then the fallback isn't used at all and we wouldn't need to attach the event listener.


function handleResize(): void {
const isMobileWidth = window.innerWidth < 480
setIsMobile(isMobileWidth)
Copy link
Contributor

Choose a reason for hiding this comment

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

my only suggestion here would be to only call setIsMobile if the value has changed, to avoid extraneous state updates

}

handleResize()

window.addEventListener('resize', handleResize)
return (): void => window.removeEventListener('resize', handleResize)
}, [])

const onToggle = (index: number): void => {
setSectionsOpenState((prevIsOpen) => {
const newIsOpen = Array(nestedLinks.length).fill(false)
// eslint-disable-next-line security/detect-object-injection
newIsOpen[index] = !prevIsOpen[index]
return newIsOpen
})
}

return (
<div className={classes}>
{nestedLinks.map((links, i) => (
<div
key={`linkSection-${i}`}
className="mobile-lg:grid-col-6 desktop:grid-col-3">
<Section
onToggle={isMobile ? (): void => onToggle(i) : undefined}
// eslint-disable-next-line security/detect-object-injection
isOpen={isMobile ? sectionsOpenState[i] : true}
haworku marked this conversation as resolved.
Show resolved Hide resolved
links={links}
/>
</div>
))}
</div>
)
}

const Section = ({
isOpen = false,
links,
onToggle,
}: {
isOpen: boolean
links: React.ReactNode[]
onToggle?: () => void
}): React.ReactElement => {
const primaryLinkOrHeading = links[0]
const secondaryLinks = links.slice(1)
haworku marked this conversation as resolved.
Show resolved Hide resolved
const classes = classnames(
'usa-footer__primary-content usa-footer__primary-content--collapsible',
{ hidden: !isOpen }
)

return (
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions
<section className={classes} onClick={onToggle} onKeyPress={onToggle}>
<h4 className="usa-footer__primary-link">{primaryLinkOrHeading}</h4>
<NavList footerSecondary items={secondaryLinks} />
</section>
)
}
33 changes: 20 additions & 13 deletions src/components/Footer/FooterNav/FooterNav.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable jsx-a11y/anchor-is-valid */
import React from 'react'

import { Footer } from '../Footer/Footer'
import { FooterNav } from './FooterNav'

export default {
Expand Down Expand Up @@ -39,19 +40,25 @@ export const MediumFooterNav = (): React.ReactElement => (
)

export const BigFooterNav = (): React.ReactElement => (
<FooterNav
aria-label="Footer navigation"
<Footer
big
links={[
['Topic', ...Array(3).fill(<a href="#">Secondary link</a>)],
[
'Topic',
<a key="2" href="#">
Secondary link that is pretty long
</a>,
...Array(2).fill(<a href="#">Secondary link</a>),
],
['Topic', ...Array(3).fill(<a href="#">Secondary link</a>)],
]}
primary={
<FooterNav
aria-label="Footer navigation"
big
links={[
['Topic', ...Array(3).fill(<a href="#">Secondary link</a>)],
[
'Topic',
<a key="2" href="#">
Secondary link that is pretty long
</a>,
...Array(2).fill(<a href="#">Secondary link</a>),
],
['Topic', ...Array(3).fill(<a href="#">Secondary link</a>)],
]}
/>
}
secondary={<></>}
/>
)
17 changes: 12 additions & 5 deletions src/components/Footer/FooterNav/FooterNav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,20 @@ describe('FooterNav component', () => {
expect(getAllByText('Primary Link').length).toBe(4)
})

it('renders extended links with "big" prop', () => {
const { container, getAllByText } = render(
it('renders extended link sections with "big" prop', () => {
const { container } = render(<FooterNav links={extendedLinks} big />)

expect(container.querySelectorAll('a').length).toBe(5)
expect(container.querySelectorAll('section').length).toBe(2)
})

it('renders list headings with "big" prop', () => {
const { container, getByText } = render(
<FooterNav links={extendedLinks} big />
)
expect(container.querySelectorAll('a').length).toBe(5)
expect(getAllByText('Purple Rain').length).toBe(3)
expect(getAllByText('Cheetah').length).toBe(2)
expect(container.querySelectorAll('h4').length).toBe(2)
expect(getByText('Types of Cats')).toBeInTheDocument()
expect(getByText('Musical Gifts')).toBeInTheDocument()
})

it('does not render extended nav links without "big" prop', () => {
Expand Down
68 changes: 16 additions & 52 deletions src/components/Footer/FooterNav/FooterNav.tsx
Original file line number Diff line number Diff line change
@@ -1,24 +1,26 @@
import React from 'react'
import {
FooterExtendedNavList,
ExtendedNavLinksType,
} from '../FooterExtendedNavList/FooterExtendedNavList'
import classnames from 'classnames'

type ExtendedNavLinks = [React.ReactNode[]]
function isExtendedNavLinks(
links: React.ReactNode[] | ExtendedNavLinksType
): links is ExtendedNavLinksType {
return (links as ExtendedNavLinksType)[0].constructor === Array
}

type FooterNavProps = {
big?: boolean
medium?: boolean
slim?: boolean
isMobile?: boolean
/*
Union type. Array of navigation links or multidimensional array of ExtendedNavLinks.
ExtendedNavLinks are ordered sub arrays that will be displayed as columns, with the first element used as the section heading.
ExtendedNavLinks can only be used with "big" prop size
Array of navigation links. Displays in simple list or an extended list with columns.
FooterExtendedNavList can only be used with multidimensional array (ExtendedNavLinksType) and "big" prop size.
*/
links: React.ReactNode[] | ExtendedNavLinks
}

function isExtendedNavLinks(
links: React.ReactNode[] | ExtendedNavLinks
): links is ExtendedNavLinks {
return (links as ExtendedNavLinks)[0].constructor === Array
links: React.ReactNode[] | ExtendedNavLinksType
}

export const FooterNav = ({
Expand All @@ -41,7 +43,9 @@ export const FooterNav = ({

return (
<nav className={navClasses} {...elementAttributes}>
{big && isExtendedNavLinks(links) && <ExtendedNav nestedLinks={links} />}
{big && isExtendedNavLinks(links) && (
<FooterExtendedNavList nestedLinks={links} />
)}

{!isExtendedNavLinks(links) && (
<ul className="grid-row grid-gap">
haworku marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -55,43 +59,3 @@ export const FooterNav = ({
</nav>
)
}

const Section = ({
links,
}: {
links: React.ReactNode[]
}): React.ReactElement => {
const primaryLinkOrHeading = links[0]
const secondaryLinks = links.slice(1)

return (
<section className="usa-footer__primary-content usa-footer__primary-content--collapsible">
<h4 className="usa-footer__primary-link">{primaryLinkOrHeading}</h4>
<ul className="usa-list usa-list--unstyled">
{secondaryLinks.map((link, i) => (
<li key={`navLink-${i}`} className="usa-footer__secondary-link">
{link}
</li>
))}
</ul>
</section>
)
}

const ExtendedNav = ({
nestedLinks,
}: {
nestedLinks: ExtendedNavLinks
}): React.ReactElement => {
return (
<div className="grid-row grid-gap-4">
{nestedLinks.map((links, i) => (
<div
key={`linkSection-${i}`}
className="mobile-lg:grid-col-6 desktop:grid-col-3">
<Section links={links} />
</div>
))}
</div>
)
}
Loading