Skip to content

Button & Link Updates #330

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

Merged
merged 15 commits into from
Oct 25, 2018
Merged

Button & Link Updates #330

merged 15 commits into from
Oct 25, 2018

Conversation

emplums
Copy link

@emplums emplums commented Oct 25, 2018

  • Button gets width prop
  • Removed block prop on Button
  • Removed ButtonLink - users should use a <Link is='button'> for this use case. The model here should be Button is a UI element that is styled like a button. Link is a UI element that is styled like a link. You can change the tag as needed.

Closes #312

Merge checklist

  • Changed base branch to release branch
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@vercel
Copy link

vercel bot commented Oct 25, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@emplums emplums changed the base branch from master to q4-haunted-kittens October 25, 2018 19:34
@emplums emplums requested review from shawnbot and jonrohan October 25, 2018 19:34
src/Link.js Outdated
const hoverColor = style({
prop: 'hoverColor',
cssProperty: 'color',
key: 'colors'
})

const styledLink = styled('a')`
const Link = ({is: Tag, children, ...rest}) => {
Copy link
Author

Choose a reason for hiding this comment

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

I did this because I couldn't figure out how to pass a dynamic tag based on a prop to styled

@@ -32,6 +58,32 @@ exports[`Link renders without any props 1`] = `
}

<a
blacklist={
Copy link
Author

Choose a reason for hiding this comment

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

@shawnbot do you know why this is happened and if it's okay or not? I think it has something to do with calling styled(Link) instead of styled('a') but I'm not positive 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, but I'm 90% sure it's a system-components thing. Maybe we could use clean-tag if we just want to keep those attributes out of the resulting DOM? 🤔

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

Code looks great, thanks for this!

@emplums emplums merged commit d417697 into q4-haunted-kittens Oct 25, 2018
@emplums emplums deleted the button-updates branch October 25, 2018 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants