New button styles #16
Conversation
| @@ -16,7 +16,7 @@ export default class Button extends React.Component<ButtonProps, {}> { | |||
|
|
|||
| render(): JSX.Element { | |||
| let content = this.props.content || "Submit"; | |||
| let className = `btn ${this.props.className || "btn-default"}`; | |||
| let className = `btn${this.props.className ? ` ${this.props.className}` : ""}`; | |||
There was a problem hiding this comment.
What happens to btn-default now?
There was a problem hiding this comment.
Not needed; the default styling comes from btn.
src/stylesheets/button.scss
Outdated
| background: $blue; | ||
| margin: .5em; | ||
| border-radius: .25em; | ||
| font-size: 16px; | ||
| &.btn-danger { |
There was a problem hiding this comment.
I know this was here before, but why isn't this danger just like how success is success and not btn-success?
There was a problem hiding this comment.
Because we were originally pulling the styling from Bootstrap, where the class name is btn-danger. (But yes, that should be updated now; good catch!)
| &:not(.tab-button) { | ||
| border-radius: .25em; | ||
| margin: .5em; | ||
| &.left-align { |
There was a problem hiding this comment.
Why not just make the margin 0 for all cases and let the app decide if the button needs margin? I think that's better than having these classes, one for each of the margin style rules.
There was a problem hiding this comment.
I would strongly prefer to keep it like this; aside from the fact that at this point it is going to be SUCH A HASSLE to go through everything and individually decide whether every single button needs margins, I think it makes a LOT more sense to have a reasonable default margin that the app can override if necessary. I can't imagine that anyone actually wants a button with a 0 margin; making 0 the default just entails creating more work for whoever is using this component, which is at odds with the purpose of having a reusable button in the first place.
We could go back to having the .5em margin for everything, rather than having these classes. But I do think that having the separate classes makes things easier for whoever is using this. I like the idea that the component already has a lot of options out of the box, so that the expectation is that you can just drop it in as-is; writing your own style rules for it seems like it should be the exception, if you want something really customized. How much is this bothering you?
There was a problem hiding this comment.
It's not that it's bothering me, it's more that margin 0 is the default for other libraries. If you check normalize, carbon, uswds, and bootstrap, the default is margin 0 for their buttons and if any app needs to add or override it, they can do so. But the margin is not set to something and you add classes to make it go back down to 0. It's a pattern that is used and I suggest trying to follow it, although I do know it'll be work to update the other apps with this change. Think about it and any other arguments are welcomed to see what should be done.
There was a problem hiding this comment.
Yeah, but I assume Bootstrap and Carbon are doing that because they're thinking in terms of a grid-based system (i.e. they figure that something other than the margin is already handling the spacing), and Normalize started out just as an alternative to CSS resets--not really the same category of thing as this. Foundation, Picnic, and Sierra--which are more in line with what we're trying to do--don't follow the margin 0 pattern.
Anyway. I propose we leave this as-is for now, and revisit the conversation (ideally with input from the UX team) at the point at which other developers start trying to use these components. But if we do want to change it now, it should be in a different branch/story; if we use the default margin 0 pattern for this, then we'll want to be consistent and use it for the other components too, so this will entail going through and individually setting margins (and dealing with resulting fallout!) for a LOT of things, and creating new branches for both interface repos--way out of the scope of a branch that was only meant for adding a couple of new button styles!
src/stylesheets/button.scss
Outdated
| border-color: $gray-brown; | ||
| } | ||
| } | ||
| &.sharp { |
|
Fix the conflicts! |
…ts into new-button-styles
|
We can leave this as is for now but using margin 0 has been much easier to use than relying on classes for such styling. I wouldn’t mind going back and resetting everything to have no margin and then move on from there but I do know there was no grid system in mind when the admins were created. Just make a ticket to keep track of potential work for updating margins with UX input (specifically whenever work on the uswds design system begins).
… On May 20, 2019, at 3:29 PM, adriana-alter ***@***.***> wrote:
@adriana-alter commented on this pull request.
In src/stylesheets/button.scss:
> color: $white;
}
&:disabled {
pointer-events: none;
}
- &:not(.tab-button) {
- border-radius: .25em;
- margin: .5em;
+ &.left-align {
Yeah, but I assume Bootstrap and Carbon are doing that because they're thinking in terms of a grid-based system (i.e. they figure that something other than the margin is already handling the spacing), and Normalize started out just as an alternative to CSS resets--not really the same category of thing as this. Foundation, Picnic, and Sierra--which are more in line with what we're trying to do--don't follow the margin 0 pattern.
Anyway. I propose we leave this as-is for now, and revisit the conversation (ideally with input from the UX team) at the point at which other developers start trying to use these components. But if we do want to change it now, it should be in a different branch/story; if we use the default margin 0 pattern for this, then we'll want to be consistent and use it for the other components too, so this will entail going through and individually setting margins (and dealing with resulting fallout!) for a LOT of things, and creating new branches for both interface repos--way out of the scope of a branch that was only meant for adding a couple of new button styles!
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
You forgot to update |
|
Ok--ticket is here. |
Add new button styles to Storybook
No description provided.