-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update Button docs #382
Update Button docs #382
Conversation
Good point. I'm inclined to leave the style changes and just clarify Some additional updates to the readme that would be good, if you have time, are:
We're hoping to wrap up and merge this changes in for Monday, when we'll be reviewing and testing all the v10 changes. If you don't have time to work on this no worries at all, I'll add the example to the readme docs. |
Sounds good 👍
Happy to make those changes! Incoming soon 💪 |
@broccolini this should be good to re-review/merge! I'm back in Slack (multi-channel, can't join design-systems) so feel free to ping me there too if you need anything on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one change and then this will be good to go :) Doc changes look great, and thanks for adding the storybook story!
@@ -42,41 +42,9 @@ status: Stable | |||
|
|||
Buttons are used for **actions**, like in forms, while textual hyperlinks are used for **destinations**, or moving from one page to another. | |||
|
|||
#### Buttons | |||
{:toc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
modules/primer-buttons/stories.js
Outdated
@@ -59,3 +59,8 @@ storiesOf('Button', module) | |||
<button className='btn btn-purple disabled'>disabled</button> | |||
</div> | |||
)) | |||
.add('btn-link', () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
@@ -181,12 +181,14 @@ | |||
display: inline-block; | |||
padding: 0; | |||
font-size: inherit; | |||
font-weight: $font-weight-normal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take these two style changes out since for now we're leaving btn-link
to work as it is, at least for the time being 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought you might say that! I'll revert that commit then 🙌
This reverts commit dbec2ae.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @JasonEtco 💖 - going to merge this into v10 so we can wrap up the rest of the fixes :)
This is a followup PR from #381 where we discussed adjusting the
btn-link
class for use with.btn
, since that'd be a common use case (even if it isn't howbtn-link
is intended to be used, to keep it consistent with other button styles it should be supported).This PR adds the relevant reset styles to
btn-link
and adds a story for Storybook 🎉One concern though: to keep the story consistent with the other buttons', I'd have to replicate the same reset styles on all the different pseudo-classes and
.STATE
(ex:.hover
) classes as well. I have no problem doing that, but wanted to see what y'all thought, since this would mean adding a few more nested styles to thebtn-link
class. If we could put a note somewhere that says "Don't use this with.btn
!" that could work too.Thanks 💖
/cc @primer/ds-core
Closes #381