-
Notifications
You must be signed in to change notification settings - Fork 67
Buttons: Initial pass at buttons, covering sizes, options, disabled, active, focus, and hover states #97
Buttons: Initial pass at buttons, covering sizes, options, disabled, active, focus, and hover states #97
Changes from 1 commit
88a4de6
b672e22
889e914
10efba1
12be9ed
f425e31
ff413db
765b19b
5fa9d3c
b88bbbd
5c0a7ec
ccc4bf9
e23fde7
3cb1565
e74e4db
005be5e
435dd5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/* | ||
* ========================================================================== | ||
* Buttons (Mixins) | ||
* ========================================================================== | ||
*/ | ||
|
||
@mixin ui-button($color, $bgcolor, $disabled: false) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider:
|
||
background: $bgcolor; | ||
border: 0; | ||
box-sizing: border-box; | ||
color: $color; | ||
display: inline-block; | ||
font-weight: 500; | ||
margin: .25em; | ||
text-align: center; | ||
text-decoration: none; | ||
text-transform: uppercase; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too opinionated, imho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once the jsass PR lands I will probably make that line into one of the configurable variables that the theme roller can set, for those who prefer not to have uppercase buttons. Sent from my iPhone
|
||
|
||
@if($disabled) { | ||
cursor: default; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was being covered in normalize.css Sent from my iPhone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, nevermind via email I thought you were commenting somewhere else, I shall adjust. |
||
opacity: .6; | ||
} | ||
} | ||
|
||
@mixin ui-button-sizes($size: default) { | ||
@if($size == large) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps use a Sass map lookup instead? E.g. font-size: map-get($button-heights, $size);
line-height: map-get($button-heights, $size); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. |
||
font-size: 20px; | ||
line-height: 20px; | ||
padding: .75em; | ||
} | ||
|
||
@if($size == default) { | ||
font-size: 16px; | ||
line-height: 16px; | ||
padding: .5em; | ||
} | ||
|
||
@if($size == small) { | ||
font-size: 14px; | ||
line-height: 14px; | ||
padding: .2em .5em; | ||
} | ||
|
||
@if($size == extra-small) { | ||
font-size: 12px; | ||
line-height: 12px; | ||
padding: .2em .5em; | ||
} | ||
} | ||
|
||
@mixin ui-button-states($color, $bgcolor) { | ||
&:active { | ||
background: darken($bgcolor, 20%); | ||
} | ||
&:focus { | ||
background: darken($bgcolor, 15%); | ||
} | ||
&:hover { | ||
background: darken($bgcolor, 10%); | ||
} | ||
&:disabled { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chassis' browser compatibility doesn't seem to be documented, but FYI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, consider adding On a tangentially related note, perhaps you jQuery folks would consider voicing support for https://www.w3.org/Bugs/Public/show_bug.cgi?id=28673 ? |
||
background: $bgcolor; | ||
opacity: .6; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Not IE8 compatible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Value is repeated on line 20. Perhaps use a variable to DRY things up? |
||
} | ||
} |
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.
I'm wondering if we should name the variables based on their purpose rather than color? This way when they get changed in the theme roller a person wouldn't create a purple button that is called "blue".
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.
Yeah, I should switch these to stuff like $button-primary-background, and $button-primary-color. That was something to put in their temporarily, but you're right, I should have better variable names.