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

christopherhuii · 2020-06-17T20:39:05Z

Summary

Currently, the Button component only allows it to be a HTML button. This PR adds an as prop to allow tags and props to be configurable.

For example:

// Button
<Button type="submit" onClick={() => {}}>
    Submit
</Button>

// Anchor Tag - Needs href and can accept other props for further configuration
<Button
    component="a"
    href="https://truss.works"
    target="_blank"
    rel="noopener noreferrer"
>
    Visit Truss
</Button>

// React Router Link - Needs "to" prop for navigation
<Button component={Link} to="/login">Login</Button>

Related Issues or PRs

Closes #161

How To Test

View code, tests, and stories

Screenshots (optional)

…tton-tag-prop-161

…utton-tag-prop-161

haworku

Nice to have an example/stories to poke around with!! Thanks for contributing, this is going to be very useful.

Made some small comments but my concern (seeing the story examples and having more context now) is around losing type safety. Would love to have more discussion of this.

haworku · 2020-06-18T14:04:16Z

src/components/Button/Button.tsx

 import classnames from 'classnames'
 import { deprecationWarning } from '../../deprecation'

 interface ButtonProps {
-  type: 'button' | 'submit' | 'reset'
-  disabled?: boolean
+  as?: FunctionComponent<any> | ComponentClass<any> | string


Suggestion - use typescript unknowninstead of any here.

haworku · 2020-06-18T14:05:00Z

src/components/Button/Button.tsx

@@ -22,14 +21,14 @@ interface ButtonProps {
  small?: boolean
  icon?: boolean
  unstyled?: boolean
+  [x: string]: any


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.

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.

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

haworku · 2020-06-18T14:16:27Z

src/components/Button/Button.tsx

 import classnames from 'classnames'
 import { deprecationWarning } from '../../deprecation'

 interface ButtonProps {
-  type: 'button' | 'submit' | 'reset'


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

haworku · 2020-06-18T14:55:36Z

src/components/Button/Button.test.tsx

@@ -12,6 +12,7 @@ describe('Button component', () => {

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


console.leftovers

haworku · 2020-06-18T15:24:21Z

src/components/Button/Button.stories.tsx

@@ -75,3 +75,30 @@ export const customClass = (): React.ReactElement => (
    Click Me
  </Button>
 )
+
+export const asAnchorTag = (): React.ReactElement => (
+  <Button


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...

suzubara

I think we discussed this in Slack but I didn't actually formally comment here. I think the best way to handle this use case would be to create a <Link> component (that takes a prop so it can be styled as a button) instead of modifying the Button component to render a different kind of element.

haworku · 2020-06-29T16:31:40Z

Changed status to WIP since what happens with #263 will influence the approach here.

suzubara · 2020-07-10T20:59:36Z

@christopherhuii @haworku @ahobson think we can close this in favor of the Link component work?

haworku · 2020-07-13T16:35:14Z

@suzubara Oh, are you are satisfied with Link to meet #161? If so, lets close both!!! Otherwise I was thinking we might still adapt Button to pass its styles to custom components ( but using the approach from Link that also preserves some type safety).

Suzanne Rozier and others added 4 commits June 3, 2020 15:47

chore(release): 1.5.0

a788d57

Merge branch 'master' of github.com:trussworks/react-uswds into ch-bu…

1cfca11

…tton-tag-prop-161

add as prop so button can be rendered with different markup

513a3f2

Merge branch 'develop' of github.com:trussworks/react-uswds into ch-b…

07d48db

…utton-tag-prop-161

christopherhuii requested review from gidjin, haworku, sarboc, suzubara and tinyels as code owners June 17, 2020 20:39

haworku reviewed Jun 18, 2020

View reviewed changes

suzubara suggested changes Jun 24, 2020

View reviewed changes

haworku added the status: wip Work in progress - not ready for code review label Jun 29, 2020

haworku changed the title ~~feat(Button): add "as" prop to allow custom tags~~ [WIP] feat(Button): add "as" prop to allow custom tags Jun 29, 2020

haworku closed this Jul 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

christopherhuii commented Jun 17, 2020

haworku left a comment •

edited

Loading

haworku Jun 18, 2020

haworku Jun 18, 2020

christopherhuii Jun 18, 2020

ahobson Jun 24, 2020

haworku Jun 18, 2020 •

edited

Loading

haworku Jun 18, 2020

haworku Jun 18, 2020 •

edited

Loading

suzubara left a comment

haworku commented Jun 29, 2020

suzubara commented Jul 10, 2020

haworku commented Jul 13, 2020 •

edited

Loading

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

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

Conversation

christopherhuii commented Jun 17, 2020

Summary

Related Issues or PRs

How To Test

Screenshots (optional)

haworku left a comment • edited Loading

Choose a reason for hiding this comment

haworku Jun 18, 2020

Choose a reason for hiding this comment

haworku Jun 18, 2020

Choose a reason for hiding this comment

christopherhuii Jun 18, 2020

Choose a reason for hiding this comment

ahobson Jun 24, 2020

Choose a reason for hiding this comment

haworku Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

haworku Jun 18, 2020

Choose a reason for hiding this comment

haworku Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

suzubara left a comment

Choose a reason for hiding this comment

haworku commented Jun 29, 2020

suzubara commented Jul 10, 2020

haworku commented Jul 13, 2020 • edited Loading

haworku left a comment •

edited

Loading

haworku Jun 18, 2020 •

edited

Loading

haworku Jun 18, 2020 •

edited

Loading

haworku commented Jul 13, 2020 •

edited

Loading