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!: Rename title props for Alert, Header and Dialog and require it for Dialog #1251

Merged
merged 15 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from 9 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
2 changes: 1 addition & 1 deletion packages/css/src/components/alert/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ There are four types of notifications:
The grid’s white space is a good reference – place the Alert in its own cell.
- By default, the Alert cannot be closed.
This functionality can be added optionally.
- Optionally, the title can be omitted.
- Optionally, the heading can be omitted.
2 changes: 1 addition & 1 deletion packages/css/src/components/header/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

# Header

Arranges the City’s logo, the title of the website, and a page menu.
Arranges the City’s logo, the title for the application, and a page menu.

## Guidelines

Expand Down
4 changes: 2 additions & 2 deletions packages/css/src/components/header/header.scss
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@
}
}

.ams-header__title {
.ams-header__app-name {
flex: 1 1 100%;

@media screen and (min-width: $ams-breakpoint-wide) {
min-inline-size: 0;
order: 2;

.ams-header__title-heading {
.ams-header__app-name-heading {
display: block;
inline-size: 100%;
line-height: 1;
Expand Down
6 changes: 6 additions & 0 deletions packages/react/src/Alert/Alert.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ describe('Alert', () => {
expect(ref.current).toBe(component)
})

it('renders a heading', () => {
const { getByText } = render(<Alert heading="Alert Title" />)

expect(getByText('Alert Title')).toBeInTheDocument()
VincentSmedinga marked this conversation as resolved.
Show resolved Hide resolved
})

it('renders the close button', () => {
const { container } = render(<Alert closeable={true} />)

Expand Down
14 changes: 7 additions & 7 deletions packages/react/src/Alert/Alert.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ export type AlertProps = {
closeable?: boolean
/** The label for the button that dismisses the Alert. */
closeButtonLabel?: string
/** The text for the Heading. */
heading?: string
/** The hierarchical level of the Alert’s heading within the document. */
headingLevel?: HeadingProps['level']
/** A function to run when dismissing. */
onClose?: () => void
/** The significance of the message conveyed. */
severity?: 'error' | 'info' | 'success' | 'warning'
/** The text for the Heading. */
title?: string
} & PropsWithChildren<HTMLAttributes<HTMLDivElement>>

const iconSvgBySeverity = {
Expand All @@ -41,26 +41,26 @@ export const Alert = forwardRef(
className,
closeable,
closeButtonLabel = 'Sluiten',
heading,
headingLevel = 2,
onClose,
severity = 'warning',
title,
...restProps
}: AlertProps,
ref: ForwardedRef<HTMLDivElement>,
) => {
const alertSize = title ? 'level-4' : 'level-5'
const Tag = title ? 'section' : 'div'
const alertSize = heading ? 'level-4' : 'level-5'
const Tag = heading ? 'section' : 'div'

return (
<Tag {...restProps} ref={ref} className={clsx('ams-alert', severity && `ams-alert--${severity}`, className)}>
<div className="ams-alert__icon">
<Icon size={alertSize} svg={iconSvgBySeverity[severity]} />
</div>
<div className="ams-alert__content">
{title && (
{heading && (
<Heading level={headingLevel} size="level-4">
{title}
{heading}
</Heading>
)}
{children}
Expand Down
24 changes: 12 additions & 12 deletions packages/react/src/Dialog/Dialog.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import '@testing-library/jest-dom'

describe('Dialog', () => {
it('renders', () => {
render(<Dialog open />)
render(<Dialog heading="Dialog Title" open />)

const component = screen.getByRole('dialog')

Expand All @@ -14,15 +14,15 @@ describe('Dialog', () => {
})

it('renders a design system BEM class name', () => {
render(<Dialog />)
render(<Dialog heading="Dialog Title" />)

const component = screen.getByRole('dialog', { hidden: true })

expect(component).toHaveClass('ams-dialog')
})

it('renders an additional class name', () => {
render(<Dialog className="extra" />)
render(<Dialog heading="Dialog Title" className="extra" />)

const component = screen.getByRole('dialog', { hidden: true })

Expand All @@ -34,56 +34,56 @@ describe('Dialog', () => {
it('supports ForwardRef in React', () => {
const ref = createRef<HTMLDialogElement>()

render(<Dialog ref={ref} />)
render(<Dialog heading="Dialog Title" ref={ref} />)

const component = screen.getByRole('dialog', { hidden: true })

expect(ref.current).toBe(component)
})

it('is not visible when open attribute is not used', () => {
render(<Dialog />)
render(<Dialog heading="Dialog Title" />)

const component = screen.getByRole('dialog', { hidden: true })

expect(component).toBeInTheDocument()
expect(component).not.toBeVisible()
})

it('renders a title', () => {
const { getByText } = render(<Dialog title="Dialog Title" />)
it('renders a heading', () => {
const { getByText } = render(<Dialog heading="Dialog Title" />)

expect(getByText('Dialog Title')).toBeInTheDocument()
VincentSmedinga marked this conversation as resolved.
Show resolved Hide resolved
})

it('renders children', () => {
const { getByText } = render(<Dialog>Dialog Content</Dialog>)
const { getByText } = render(<Dialog heading="Dialog Title">Dialog Content</Dialog>)

expect(getByText('Dialog Content')).toBeInTheDocument()
})

it('renders actions when provided', () => {
const { getByText } = render(<Dialog actions={<button>Click Me</button>} />)
const { getByText } = render(<Dialog heading="Dialog Title" actions={<button>Click Me</button>} />)

expect(getByText('Click Me')).toBeInTheDocument()
})

it('does not render actions when not provided', () => {
const { queryByText } = render(<Dialog />)
const { queryByText } = render(<Dialog heading="Dialog Title" />)

expect(queryByText('Click Me')).not.toBeInTheDocument()
})

it('renders DialogClose button', () => {
render(<Dialog open />)
render(<Dialog heading="Dialog Title" open />)

const closeButton = screen.getByRole('button', { name: 'Sluiten' })

expect(closeButton).toBeInTheDocument()
})

it('renders a custom close label', () => {
render(<Dialog open closeButtonLabel="Close" />)
render(<Dialog heading="Dialog Title" open closeButtonLabel="Close" />)

const closeButton = screen.getByRole('button', { name: 'Close' })

Expand Down
6 changes: 4 additions & 2 deletions packages/react/src/Dialog/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,19 @@ export type DialogProps = {
actions?: ReactNode
/** The label for the button that dismisses the Dialog. */
closeButtonLabel?: string
/** The text for the Heading. */
heading: string
} & PropsWithChildren<DialogHTMLAttributes<HTMLDialogElement>>

export const Dialog = forwardRef(
(
{ actions, children, className, closeButtonLabel = 'Sluiten', title, ...restProps }: DialogProps,
{ actions, children, className, closeButtonLabel = 'Sluiten', heading, ...restProps }: DialogProps,
ref: ForwardedRef<HTMLDialogElement>,
) => (
<dialog {...restProps} ref={ref} className={clsx('ams-dialog', className)}>
<form method="dialog" className="ams-dialog__form">
<header className="ams-dialog__header">
<Heading size="level-4">{title}</Heading>
<Heading size="level-4">{heading}</Heading>
<IconButton formNoValidate label={closeButtonLabel} size="level-4" />
</header>
<article className="ams-dialog__article">{children}</article>
Expand Down
11 changes: 11 additions & 0 deletions packages/react/src/Header/Header.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ describe('Header', () => {
expect(logoLinkTitle).toHaveTextContent('Go to homepage')
})

it('renders an application name', () => {
render(<Header appName="Application name" />)

const heading = screen.getByRole('heading', {
name: 'Application name',
level: 1,
})

expect(heading).toBeInTheDocument()
})

it('renders with links', () => {
const { container } = render(<Header links={<div>Menu Content</div>} />)

Expand Down
20 changes: 10 additions & 10 deletions packages/react/src/Header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,29 @@ import type { LogoBrand } from '../Logo'
import { VisuallyHidden } from '../VisuallyHidden'

export type HeaderProps = {
/** The name of the application. */
appName?: string
/** The list of menu links. Use a Page Menu here. */
links?: ReactNode
/** The name of the brand for which to display the logo. */
logoBrand?: LogoBrand
/** The url for the link on the logo. */
logoLink?: string
/** The accessible text for the link on the logo. */
logoLinkTitle?: string
/** The name of the application. */
title?: string
/** The list of menu links. Use a Page Menu here. */
links?: ReactNode
/** A button to toggle the visibility of a Mega Menu. */
menu?: ReactNode
} & HTMLAttributes<HTMLElement>

export const Header = forwardRef(
(
{
appName,
className,
links,
logoBrand = 'amsterdam',
logoLink = '/',
logoLinkTitle = 'Ga naar de homepage',
title,
links,
menu,
...restProps
}: HeaderProps,
Expand All @@ -49,10 +49,10 @@ export const Header = forwardRef(
</a>
{links && <div className="ams-header__links">{links}</div>}
{menu && <div className="ams-header__menu">{menu}</div>}
{title && (
<div className="ams-header__title">
<Heading level={1} size="level-3" className="ams-header__title-heading">
{title}
{appName && (
<div className="ams-header__app-name">
<Heading level={1} size="level-3" className="ams-header__app-name-heading">
{appName}
</Heading>
</div>
)}
Expand Down
4 changes: 2 additions & 2 deletions storybook/src/components/Alert/Alert.docs.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ For clarification, formatted text can be included in the alert.

<Canvas of={AlertStories.WithList} />

### Without Title
### Without Heading

Sometimes, a title is unnecessary.
The icon automatically becomes slightly smaller.

<Canvas of={AlertStories.WithoutTitle} />
<Canvas of={AlertStories.WithoutHeading} />
14 changes: 7 additions & 7 deletions storybook/src/components/Alert/Alert.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ const meta = {
title: 'Components/Feedback/Alert',
component: Alert,
args: {
title: 'Let op',
closeable: false,
heading: 'Let op',
},
} satisfies Meta<typeof Alert>

Expand All @@ -33,7 +33,7 @@ export const Default: Story = {
export const Warning: Story = {
args: {
children: <Paragraph>U bent vergeten verplichte velden in te vullen.</Paragraph>,
title: 'Vul de gegevens aan',
heading: 'Vul de gegevens aan',
},
}

Expand All @@ -45,17 +45,17 @@ export const Error: Story = {
Probeer het over een paar minuten opnieuw.
</Paragraph>
),
heading: 'Niet gelukt',
severity: 'error',
title: 'Niet gelukt',
},
}

export const Success: Story = {
args: {
children: <Paragraph>Het formulier is verzonden. We hebben uw gegevens goed ontvangen.</Paragraph>,
closeable: true,
heading: 'Gelukt',
severity: 'success',
title: 'Gelukt',
},
}

Expand All @@ -74,14 +74,14 @@ export const Info: Story = {

export const WithList: Story = {
args: {
title: 'Vul de gegevens aan',
children: [
<Paragraph key={1}>U bent vergeten de volgende verplichte velden in te vullen:</Paragraph>,
<UnorderedList key={2}>
<UnorderedList.Item>Naam</UnorderedList.Item>
<UnorderedList.Item>Telefoonnummer</UnorderedList.Item>
</UnorderedList>,
],
heading: 'Vul de gegevens aan',
},
}

Expand All @@ -100,13 +100,13 @@ export const WithInlineLink: Story = {
},
}

export const WithoutTitle: Story = {
export const WithoutHeading: Story = {
args: {
children: (
<Paragraph>
De geschatte wachttijd in de adreszoeker klopt momenteel niet altijd. We werken aan een oplossing.
</Paragraph>
),
title: undefined,
heading: undefined,
},
}
Loading
Loading