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

Jump to bottom

feat: New Component SiteAlert #1099

Merged
merged 29 commits into from
Apr 26, 2021
Merged

feat: New Component SiteAlert #1099

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ac34b15
Add files, render some text in a div in Storybook, add one test
SirenaBorracha Apr 6, 2021
c1d88db
Add minimum Storybook examples, add test to check for application of …
SirenaBorracha Apr 6, 2021
8fa9302
Update Storybook examples to match USWDS, add test data specific to e…
SirenaBorracha Apr 6, 2021
379cbbf
Make Storybook examples match those in USWDS
SirenaBorracha Apr 6, 2021
46bd6f7
Add class name to Storybook links so they render the correct color
SirenaBorracha Apr 6, 2021
255d10d
Additional conditional for not applying no-header to slim site alert
SirenaBorracha Apr 6, 2021
61f326a
EsLint disable anchor check, update anchors with simplified href values
SirenaBorracha Apr 6, 2021
1fcd8f3
Add test checking that the component renders with both slim and no-ic…
SirenaBorracha Apr 6, 2021
6d06203
Fix conditional application of no-heading class so that it is not app…
SirenaBorracha Apr 7, 2021
08f2d38
Update Storybook withList examples with custom lists
SirenaBorracha Apr 7, 2021
a1d396d
Export SiteAlert from index.ts
SirenaBorracha Apr 7, 2021
bf5355b
Expand on SiteAlert tests
SirenaBorracha Apr 7, 2021
c90bd6a
Respond to PR feedback, add test for overwriting the aria-label when …
SirenaBorracha Apr 7, 2021
5454a4e
Missed a comment on the PR; update anchors to use internal Link compo…
SirenaBorracha Apr 7, 2021
d1c50cc
Factor query into single variable, add test checking for passed in he…
SirenaBorracha Apr 8, 2021
0f0cdb2
Attempt at adding controls for slim and showIcon styling differences …
SirenaBorracha Apr 8, 2021
69737ff
Use storybook controls to add storybook example Alert With Custom Con…
SirenaBorracha Apr 8, 2021
6f658dd
Use Link instead of anchor in .stories and .tests, update classNames …
SirenaBorracha Apr 8, 2021
2b66fcf
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 9, 2021
87de42f
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 12, 2021
bb32c87
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 15, 2021
c164a87
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 19, 2021
8aab95c
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 20, 2021
6e1db38
Remove whitespace from package.json
SirenaBorracha Apr 23, 2021
eb7d678
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 23, 2021
485b301
Merge branch 'main' into ak-new-component-site-alert-981
SirenaBorracha Apr 23, 2021
7315713
Remove weird whitespace addition to yarn.lock
SirenaBorracha Apr 26, 2021
a1e709f
Merge branch 'ak-new-component-site-alert-981' of github.com:trusswor…
SirenaBorracha Apr 26, 2021
9a023c6
Merge branch 'main' into ak-new-component-site-alert-981
brandonlenz Apr 26, 2021
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
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"peerDependencies": {
"react": "^16.x || ^17.x",
"react-dom": "^16.x || ^17.x",
"uswds": "2.9.0"
"uswds": "2.10.0"
},
"devDependencies": {
"@babel/core": "^7.10.5",
Expand Down Expand Up @@ -126,7 +126,7 @@
"ts-jest": "^26.1.2",
"typescript": "^3.8.0",
"url-loader": "^4.0.0",
"uswds": "2.9.0",
"uswds": "2.10.0",
"webpack": "^4.41.0",
"webpack-cli": "^4.0.0"
},
Expand Down
157 changes: 157 additions & 0 deletions src/components/SiteAlert/SiteAlert.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
/* eslint-disable jsx-a11y/anchor-is-valid */

import React from 'react'

import { SiteAlert } from './SiteAlert'
import { Link } from '../Link/Link'

export default {
title: 'Components/SiteAlert',
component: SiteAlert,
parameters: {
docs: {
description: {
component: `
### USWDS 2.0 SiteAlert component

Source: http://designsystem.digital.gov/components/site-alert
`,
},
},
},
}

const infoHeading = 'Short alert message'

const additionalContext = (
<p className="usa-alert__text">
Additional context and followup information including{' '}
<Link className="usa-link" href="#">
brandonlenz marked this conversation as resolved.
Show resolved Hide resolved
a link
</Link>
.
</p>
)

const emergencyHeading = 'Emergency alert message'

const infoWithList = (
<ul className="usa-list">
<li>
The primary informational message and{` `}
<Link className="usa-link" href="#">
a link
</Link>
{` `}for supporting context.
</li>
<li>
Another message,{` `}
<Link className="usa-link" href="#">
and another link
</Link>
.
</li>
<li>A final informational message.</li>
</ul>
)

const emergencyWithList = (
<ul className="usa-list">
<li>
The primary emergency message and{` `}
<Link className="usa-link" href="#">
a link
</Link>
{` `}for supporting context.
</li>
<li>
Another message,{` `}
<Link className="usa-link" href="#">
and another link
</Link>
.
</li>
<li>A final emergency message.</li>
</ul>
)

const shortAlertContent = (
<p className="usa-alert__text">
Copy link
Contributor

Choose a reason for hiding this comment

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

At initial glance, this <p> is something I would want to encapsulate similar to #1100 (generifying the usa-alert__text class into an AlertText component could be used for a child of both Alert and SiteAlert). That can be an enhancement.

I'm also thinking about how Alert (which is very similar) is very rigid, while SiteAlert has this flexible approach. I think the SiteAlert approach is in line with our current design philosophy.

Does that then mean we should add a backlog item for a breaking release where we change Alert to be a bit more flexible like SiteAlert, and allow things like lists to be passed in? Mostly just thinking out loud and noticing some things that would be nice for us to decide how to be consistent about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call out. Another idea: I could see the children of SiteAlert (and honestly Alert as well) accepting children of type string | JSX.element. If a string is passed in we put the string in a <p> tag with the usa-alert class) but if a component is passed in we just display that component. What do folks think of that approach?

In terms of refactoring Alert - I think that could be a good task for the backlog. It probably should be handling flexible content. Right now it uses the validation prop to adjust classes but also to be more flexible (validations are lists) which is quite weird. I think I wrote that code too 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that approach! We might need to expand to accept string | JSX.Element | JSX.Element[] in the event of multiple child elements.

I think the only advantage of making the <AlertText /> subcomponent is for the scenario where someone wanted to do something like:

<SiteAlert>
  <AlertText>Here's some text about the list below:<AlertText />
  <ul className="usa-list">
    <li>something</li>
    <li>something else</li>
  </ul>
<SiteAlert>

The string | JSX.Element | JSX.Element[] approach doesn't preclude that so we could proceed that route and always leave AlertText as an enhancement should the need arise.

Copy link
Contributor

Choose a reason for hiding this comment

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

The string | JSX.Element | JSX.Element[] approach doesn't preclude that so we could proceed that route and always leave AlertText as an enhancement should the need arise.

Nice.

<strong>Short alert message.</strong>
&nbsp;Additional context and followup information including&nbsp;
<Link className="usa-link" href="#">
a link
</Link>
.
</p>
)

export const standardInformationalSiteAlert = (): React.ReactElement => (
<SiteAlert variant="info" heading={infoHeading}>
{additionalContext}
</SiteAlert>
)

export const standardEmergencySiteAlert = (): React.ReactElement => (
<SiteAlert variant="emergency" heading={emergencyHeading}>
{additionalContext}
</SiteAlert>
)

export const informationalAlertWithNoHeader = (): React.ReactElement => (
<SiteAlert variant="info">{shortAlertContent}</SiteAlert>
)

export const emergencyAlertWithNoHeader = (): React.ReactElement => (
<SiteAlert variant="emergency">{shortAlertContent}</SiteAlert>
)

export const informationalAlertWithList = (): React.ReactElement => (
<SiteAlert variant="info" heading={infoHeading}>
{infoWithList}
</SiteAlert>
)

export const emergencyAlertWithList = (): React.ReactElement => (
<SiteAlert
variant="emergency"
heading={emergencyHeading}
aria-label="Site alert">
{emergencyWithList}
</SiteAlert>
)

export const informationalAlertNoIcon = (): React.ReactElement => (
<SiteAlert variant="info" showIcon={false}>
{shortAlertContent}
</SiteAlert>
)
export const emergencyAlertNoIcon = (): React.ReactElement => (
<SiteAlert variant="emergency" showIcon={false}>
{shortAlertContent}
</SiteAlert>
)

export const slimInformationalAlert = (): React.ReactElement => (
<SiteAlert variant="info" slim>
{shortAlertContent}
</SiteAlert>
)

export const slimEmergencyAlert = (): React.ReactElement => (
<SiteAlert variant="emergency" slim>
{shortAlertContent}
</SiteAlert>
)

export const slimInformationalAlertNoIcon = (): React.ReactElement => (
<SiteAlert variant="info" slim showIcon={false}>
{shortAlertContent}
</SiteAlert>
)

export const slimEmergencyAlertNoIcon = (): React.ReactElement => (
<SiteAlert variant="emergency" slim showIcon={false}>
{shortAlertContent}
</SiteAlert>
)
124 changes: 124 additions & 0 deletions src/components/SiteAlert/SiteAlert.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/* eslint-disable jsx-a11y/anchor-is-valid, react/jsx-key */
import React from 'react'
import { render } from '@testing-library/react'

import { SiteAlert } from './SiteAlert'

const testChildren = (
<p className="usa-alert__text">
some default text&nbsp;
<a className="usa-link" href="#">
with a link
</a>
.
</p>
)

const testDefaultProps = {
heading: 'test heading',
children: testChildren,
}

const testChildrenWithList = (
<ul>
<li>
some default text&nbsp;
<a className="usa-link" href="#">
with a link&nbsp;
</a>
</li>
<li>
another list item&nbsp;
<a className="usa-link" href="#">
with a link
</a>
</li>
<li>another list item, no link</li>
</ul>
)

describe('SiteAlert component', () => {
it('renders without errors', () => {
const { getByTestId, getByRole } = render(
<SiteAlert variant="info" {...testDefaultProps} />
)

expect(getByTestId('siteAlert')).toBeInTheDocument()
expect(getByRole('link')).toBeInTheDocument()
})

it('accepts a className', () => {
const { getByTestId } = render(
<SiteAlert
variant="info"
className="custom-class-name"
{...testDefaultProps}
/>
)

expect(getByTestId('siteAlert')).toHaveClass(
'usa-site-alert usa-site-alert--info custom-class-name'
)
})

it('accepts a custom aria-label', () => {
const { getByTestId } = render(
<SiteAlert
variant="emergency"
aria-label="custom aria label"
{...testDefaultProps}
/>
)

expect(getByTestId('siteAlert')).toHaveAttribute(
'aria-label',
'custom aria label'
)
})

it('renders a passed in heading', () => {
const { getByRole } = render(
<SiteAlert variant="info" {...testDefaultProps} />
)

expect(getByRole('heading')).toBeInTheDocument()
expect(getByRole('heading')).toHaveClass('usa-alert__heading')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: This test could include a check that the heading text is indeed rendered. Right now we just check that a heading exists, but not its text content.

Copy link
Contributor

@brandonlenz brandonlenz Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ooo and a quick follow up on these lines since I didn't notice on my first pass:

@SirenaBorracha if you ever find yourself doing getBy___ or queryBy___ for the same thing multiple times in a test you can factor that out into a const, e.g:

const heading = getByRole('heading')
expect(heading).toBeInTheDocument()
expect(heading).toHaveClass('usa-alert__heading')

I'm not sure if the performance impact of querying the dom for a specific element is high, but its an easy optimization we can do to keep the tests efficient.

})

it('renders emergency site alert without errors', () => {
const { getByTestId } = render(
<SiteAlert variant="emergency" {...testDefaultProps} />
)

expect(getByTestId('siteAlert')).toHaveClass(
'usa-site-alert usa-site-alert--emergency'
)
})

it('renders passed in link', () => {
const { getByRole } = render(
<SiteAlert variant="info">{testChildren}</SiteAlert>
)

expect(getByRole('link')).toBeInTheDocument()
expect(getByRole('link')).toHaveClass('usa-link')
})

it('renders a passed in list', () => {
const { getAllByRole } = render(
<SiteAlert variant="emergency">{testChildrenWithList}</SiteAlert>
)
expect(getAllByRole('link')).toHaveLength(2)
})

it('renders slim and no icon when passed both', () => {
const { getByTestId } = render(
<SiteAlert variant="info" slim={true} showIcon={false}>
{testChildren}
</SiteAlert>
)
expect(getByTestId('siteAlert')).toHaveClass(
'usa-site-alert usa-site-alert--info usa-site-alert--no-icon usa-site-alert--slim'
)
})
})
49 changes: 49 additions & 0 deletions src/components/SiteAlert/SiteAlert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React from 'react'
import classnames from 'classnames'

interface SiteAlertProps {
variant: 'info' | 'emergency'
children: React.ReactNode
heading?: string
showIcon?: boolean
slim?: boolean
className?: string
}

export const SiteAlert = ({
variant,
children,
heading,
showIcon = true,
slim = false,
className,
...sectionProps
}: SiteAlertProps & JSX.IntrinsicElements['section']): React.ReactElement => {
const classes = classnames(
'usa-site-alert',
{
'usa-site-alert--info': variant === 'info',
'usa-site-alert--emergency': variant === 'emergency',
'usa-site-alert--no-heading': heading === undefined && !slim,
brandonlenz marked this conversation as resolved.
Show resolved Hide resolved
'usa-site-alert--no-icon': !showIcon,
'usa-site-alert--slim': slim,
},
className
)
return (
<section
data-testid="siteAlert"
className={classes}
aria-label="Site alert"
{...sectionProps}>
<div className="usa-alert">
<div className="usa-alert__body">
{heading && <h3 className="usa-alert__heading">{heading}</h3>}
{children}
</div>
</div>
</section>
)
}

export default SiteAlert
2 changes: 2 additions & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export { BreadcrumbLink } from './components/breadcrumb/BreadcrumbLink/Breadcrum

export { Search } from './components/Search/Search'

export { SiteAlert } from './components/SiteAlert/SiteAlert'

/** Truss-designed components */
export {
Modal,
Expand Down
Loading