Button and ButtonLink ✨ #30
Conversation
src/Button.js
Outdated
| 'btn-primary': type === 'primary', | ||
| 'btn-danger': type === 'danger', | ||
| 'btn-outline': type === 'outline', | ||
| 'btn-block': block, |
There was a problem hiding this comment.
I know a lot of people like do this with buttons because it feels good to dry up the code, but I think it actually makes a simpler api to keep buttons separate around their theme. It does mean there are more components to know about, but it means there are fewer props to use with a single button component.
Instead of <Button primary block large/> you'd do <ButtonPrimary block large /> etc.
It also means we can be very intentional with which props we make available to which button themes. Later (which we will!) when we want to add new button variations or props (especially for marketing), we can be more deliberate and choose what props are available for each button. For example, we might never want to use an Octicon in a ButtonOutline or ButtonDanger but we do for primary and etc. (just using examples here).
I think there are definitely cases where we want color themes to be props such as for States, but buttons are pretty complex and we're likely to add onto this component.
package.json
Outdated
| @@ -13,15 +18,12 @@ | |||
| "author": "GitHub, Inc.", | |||
| "contributors": [ | |||
| "Diana Mounter <diana.mounter@gmail.com>", | |||
| "Shawn Allen <shawn.allen@github.com>" | |||
| "Shawn Allen <shawn.allen@github.com>", | |||
There was a problem hiding this comment.
Lol so I totally copy-pasted this from another project and did not mean to include. Do y'all want to us to list contributors here? If we do, does that mean we update when anyone else contributes too? I'm worried that could get hairy.
If you keep it please can you update my email to my github email please 😬 broccolini@github.com
There was a problem hiding this comment.
I'm 👎 on it, personally I don't want my email in there. Feels like I might get spammed
| import React from 'react' | ||
| import classnames from 'classnames' | ||
|
|
||
| const ButtonSecondary = props => ( |
There was a problem hiding this comment.
I didn't see any secondary button classes in primer-buttons so removing this one, I don't think secondary buttons exist yet 🤷♀️
There was a problem hiding this comment.
It's exactly the same as Button 😑 which is not really ideal, we can revisit this later as I think people are used to using Secondary for this style.
This PR adds Button and ButtonLink 🔢 🔗
Button and ButtonLink could just be Button with a prop to render an tag instead of a tag, but because Button can also take a linkStyle prop to be styled like a link, I think the two should stay separate to avoid confusion. They also render different elements so they feel good as separate components.
From a UX/a11y perspective, usage of ButtonLink should probably be discouraged when possible. Navigational items should be links and look like links, action items should be buttons and look like buttons.
Some notes I wrote up while designing the API for primer-buttons: https://docs.google.com/document/d/1YP2IcysUL0hHMOqPuDOaUka1i7vfnKVPYHwsTZgn7Ac/edit
Part of 0.0.1 beta release: #24