Skip to content
This repository has been archived by the owner on Dec 11, 2021. It is now read-only.

Buttons: Initial pass at buttons, covering sizes, options, disabled, active, focus, and hover states #97

Closed
wants to merge 17 commits into from

Conversation

sfrisk
Copy link
Contributor

@sfrisk sfrisk commented Jul 17, 2015

This is my first initial pass at buttons. It this is the direction people want to go with for buttons, I can add a few more options for buttons (colors, button group, block button etc).

@cvrebert
Copy link
Contributor

Why so much @extending? It's arguably an antipattern.

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 17, 2015

@cvrebert As I think about it, mixins would probably have be better solution, especially if we want to add additional options. I can easily switch to using mixins instead. Part of this issue was also to test the BEM naming convention (since we haven't had any commits with that yet), and get an idea if visually this was the type of button we wanted to go with. I'll update the PR (although probably not tonight) with a mixin version.

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 17, 2015

Or you know, maybe just make things mixins tonight, since I can't sleep. Let me know what you think, and if you approach works.

<a class="ui-button--default" href="#" role="button">Link</a>
<button class="ui-button--default" type="submit">Button</button>
<input class="ui-button--default" type="button" value="Input">
<input class="ui-button--default" type="submit" value="Submit">
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a type="reset" example

@cvrebert
Copy link
Contributor

Have you considered:

  • Block-level buttons
  • A class to make a <button> look like a normal <a>
  • Additional classes to emulate the appearance of :active, :focus, :hover

margin: .25em;
text-align: center;
text-decoration: none;
text-transform: uppercase;
Copy link
Contributor

Choose a reason for hiding this comment

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

Too opinionated, imho

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

On Jul 17, 2015, at 1:48 AM, Chris Rebert [email protected] wrote:

In scss/atoms/buttons/_mixins.scss:

+/*
+* ==========================================================================
+* Buttons (Mixins)
+* ==========================================================================
+*/
+
+@mixin ui-button($color, $bgcolor, $disabled: false) {

  • background: $bgcolor;
  • border: 0;
  • color: $color;
  • display: inline-block;
  • font-weight: 500;
  • margin: .25em;
  • text-align: center;
  • text-decoration: none;
  • text-transform: uppercase;
    Too opinionated, imho


Reply to this email directly or view it on GitHub.

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 17, 2015

Block-level buttons were actually next on my list of things to add.
Making a button look like a link is a good one, I will add it to the list. Same with classes for active/hover/focus.

Sent from my iPhone

On Jul 17, 2015, at 1:47 AM, Chris Rebert [email protected] wrote:

Have you considered:

Block-level buttons
A class to make a look like a normal
Additional classes to emulate the appearance of :active, :focus, :hover`

Reply to this email directly or view it on GitHub.

&:hover {
background: darken($bgcolor, 10%);
}
&:disabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Chassis' browser compatibility doesn't seem to be documented, but FYI :disabled is not IE8 compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, consider adding &[aria-disabled="true"], which makes toggling the disabledness of an <a> button much simpler (since it's context-insensitive and uniform), and also communicates the disabledness to accessibility tools.

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 ?

@cvrebert
Copy link
Contributor

Consider implementing twbs/bootstrap#15947 as well

$chassis-blue: #4fc0c8;
$chassis-gray-light: #f2f2f2;
$chassis-gray-dark: #383838;

Copy link
Contributor

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".

Copy link
Contributor Author

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.

@cvrebert
Copy link
Contributor

Also, what about combinations of multiple states? (Example: twbs/bootstrap#16767)

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 17, 2015

@cvrebert Ooo, I hadn't thought of that one. Might need to tweak how disabled styling currently works to make that work.

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 20, 2015

Also @geekman-rohit pointed out that the primary button contrast don't actually pass accessibility contrast checking. I used colors suggested for use with the Chassis Logo, but if anyone has any alternative color suggestions that are accessible, I would love to hear them.

@geekman-rohit
Copy link
Contributor

Basically we want the background - foreground colors to have more contrast so they are easier to read to all people. We also need to see what tests it should pass? I think the primary button should pass AAA while it should be okay for disabled buttons to be AA or even A http://www.w3.org/TR/UNDERSTANDING-WCAG20/conformance.html

@arschmitz
Copy link
Contributor

there are automated tools we can use with grunt for this sort of checking its been on my todo for a while to set them up

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 20, 2015

I'll add an issue for Accessibility Checking

@sfrisk
Copy link
Contributor Author

sfrisk commented Jul 21, 2015

This was brought up in the meeting today, but should we have the default button be a "block" button, since we'll be designing this mobile first, with an optional modifier to have an inline button? Or the other way around?

@geekman-rohit
Copy link
Contributor

I think it should be inline with a modifier to make it block.

@sfrisk
Copy link
Contributor Author

sfrisk commented Aug 18, 2015

@geekman-rohit added some different types of buttons to test the performance stuff you're working on

@sfrisk
Copy link
Contributor Author

sfrisk commented Dec 15, 2015

Closing this in favor of #138, which is our new approach on this.

@sfrisk sfrisk closed this Dec 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants