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

[WIP] feat(Button): add "as" prop to allow custom tags #263

Closed
wants to merge 4 commits into from
Closed

[WIP] feat(Button): add "as" prop to allow custom tags #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a788d57
chore(release): 1.5.0
Jun 3, 2020
1cfca11
Merge branch 'master' of github.com:trussworks/react-uswds into ch-bu…
christopherhuii Jun 17, 2020
513a3f2
add as prop so button can be rendered with different markup
christopherhuii Jun 17, 2020
07d48db
Merge branch 'develop' of github.com:trussworks/react-uswds into ch-b…
christopherhuii Jun 17, 2020
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
27 changes: 27 additions & 0 deletions src/components/Button/Button.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,30 @@ export const customClass = (): React.ReactElement => (
Click Me
</Button>
)

export const asAnchorTag = (): React.ReactElement => (
<Button
Copy link
Contributor

@haworku haworku Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like an example of where using as props are any string could be an issue- there's no way to verify, for example, that they are passing types that are valid for the anchor tag. Or that they even pass a valid html element tag name as a string...

as="a"
href="https://truss.works"
target="_blank"
rel="noopener noreferrer">
Visit Truss
</Button>
)

export const asCustomComponent = (): React.ReactElement => {
interface MockLinkProps {
to: string
children: React.ReactNode
}

const CustomLink = ({ to, children }: MockLinkProps): React.ReactElement => (
<a href={to}>{children}</a>
)

return (
<Button as={CustomLink} to="https://truss.works">
Visit Truss
</Button>
)
}
47 changes: 47 additions & 0 deletions src/components/Button/Button.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ describe('Button component', () => {

it('renders without errors', () => {
const { queryByTestId } = render(<Button type="button">Click Me</Button>)
console.log(queryByTestId('button'))
Copy link
Contributor

Choose a reason for hiding this comment

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

console.leftovers

expect(queryByTestId('button')).toBeInTheDocument()
})

Expand Down Expand Up @@ -112,4 +113,50 @@ describe('Button component', () => {
expect(getByTestId('button')).toHaveClass('usa-button')
expect(getByTestId('button')).toHaveClass('customClass')
})

it('renders button as a different component', () => {
interface MockLinkProps {
to: string
children: React.ReactNode
}

const MockLink = ({ to, children }: MockLinkProps): React.ReactElement => (
<a href={to}>{children}</a>
)

const { getByTestId, container } = render(
<Button as={MockLink} to="https://truss.works">
<div data-testid="button-child" />
</Button>
)

expect(container.querySelector('a')).toBeInTheDocument()
expect(getByTestId('button-child')).toBeTruthy()
})

it('renders button as different element', () => {
const { container } = render(
<Button as="a" href="https://truss.works">
Click Me
</Button>
)

expect(container.querySelector('a')).toBeInTheDocument()
})

it('renders additional props', () => {
const { getByTestId } = render(
<Button
as="a"
href="https://truss.works"
target="_blank"
rel="noopener noreferrer">
Click Me
</Button>
)

expect(getByTestId('button')).toHaveAttribute('href', 'https://truss.works')
expect(getByTestId('button')).toHaveAttribute('target', '_blank')
expect(getByTestId('button')).toHaveAttribute('rel', 'noopener noreferrer')
})
})
28 changes: 13 additions & 15 deletions src/components/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import React from 'react'
import React, { FunctionComponent, ComponentClass } from 'react'
import classnames from 'classnames'
import { deprecationWarning } from '../../deprecation'

interface ButtonProps {
type: 'button' | 'submit' | 'reset'
Copy link
Contributor

@haworku haworku Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this being removed because of redundancy?

We have tried to start a pattern of explicitly requiring props that seem essential to the component. Here are some new docs that outline this and a related comment.

Of course, these are opinions and we are still figuring it out. I'm curious your thoughts on it @christopherhuii . cc: @suzubara

disabled?: boolean
as?: FunctionComponent<any> | ComponentClass<any> | string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion - use typescript unknowninstead of any here.

children: React.ReactNode
secondary?: boolean
base?: boolean
Expand All @@ -22,14 +21,14 @@ interface ButtonProps {
small?: boolean
icon?: boolean
unstyled?: boolean
[x: string]: any
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm... what was this to address? 🤔 Was it to broaden the created element's props more? And is this broadening of props only necessary because we accept an as prop as a string name?

Feels like we are losing type safety, that gives me pause since we are creating a library to be used in a lot of unknown contexts, We are trying to wrap styles, yes, but also try to enable good practices around accessibility, responsiveness etc - so we may need constraints (to make sure that an anchor tag "button" has a role: button for example).

If this was in an application where the use cases are known, its different.

If it solves the problem, I'd prefer us to limit as types to FunctionComponent<any> | ComponentClass<any> instead for the sake of this library. Someone can always pass in their anchor tag, already with any props/attributes that way - the functionality isn't lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this line was the best way to do this.

I think we are losing type safety. It allows the user to add whatever prop they need. In the case of anchor tags, target and rel are fairly common to add. For accessibility purposes, aria-* can be added. Users may also want to add data-*. I added it to accommodate for the various use cases a user may have. I'm not sure what the best way is to move this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my exploration of how to extend props to allow valid property names in #267 is relevant to this.

}

export const Button = (
props: ButtonProps & React.HTMLAttributes<HTMLButtonElement>
): React.ReactElement => {
const {
type,
disabled,
as,
children,
secondary,
base,
Expand All @@ -41,8 +40,8 @@ export const Button = (
small,
icon,
unstyled,
onClick,
className,
...restProps
} = props

if (big) {
Expand Down Expand Up @@ -71,15 +70,14 @@ export const Button = (
className
)

return (
<button
type={type}
className={classes}
disabled={disabled}
onClick={onClick}
data-testid="button">
{children}
</button>
return React.createElement(
as || 'button',
{
...restProps,
'data-testid': 'button',
className: classes,
},
children
)
}

Expand Down