Updated buttons to be more accessible. #373

dsjen · 2016-11-15T20:04:57Z

Description

FEDX-231

Accessility updates to buttons:

Darkened hover/focus colors
Darked link read color
Updated disabled button styling on inverse background

Preview: http://ux-test.edx.org/dsjen/a11y-buttons/components/buttons/

Manual Test Plan

Confirm color is darker when hovering over buttons.
Confirm color of links is darker when "read."
Confirm under link appears when hovering over links.
Confirm disabled button is correct in inverse background state.

Testing Checklist

Manually test responsive behavior.
Manually test right-to-left behavior.
Manually test a11y support.

Non-testing Checklist

Consider any documentation your change might need, and which users will be affected by this change.

Post-review

Squash commits into discrete sets of changes with descriptive commit messages.

Reviewers

I'm looking for two developer 👍 in addition to @chris-mike as product reviewer. If you've been tagged for review, please check your corresponding box once you've given the 👍.

@chris-mike -- Please confirm that this is what you were expecting.

* darkened colors * updated inverse disabled butotn * updated colors and read state on links

mention-bot · 2016-11-15T20:04:58Z

@dsjen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @andy-armstrong, @clrux and @talbs to be potential reviewers.

andy-armstrong · 2016-11-15T20:17:50Z

@dsjen This looks great! A couple of minor things I noticed:

the elevated buttons (which are odd at the best of times) have an odd hover color for the bottom bar
this wasn't your change, but the inverse disabled button appears to have a cut-and-paste error (it is labeled "base" and doesn't have the correct class)

thallada

I put a few comments/concerns inline.

thallada · 2016-11-15T20:43:29Z

pldoc/_components/buttons.md

@@ -93,7 +93,7 @@ info:          Buttons should be used for performing actions within the edX envi
        <button type="button" class="btn-inverse">Inverse</button>
        <button type="button" class="btn-inverse btn-large">Inverse Large</button>
        <button type="button" class="btn-inverse btn-small">Inverse Small</button>
-        <button type="button" class="btn-inverse" disabled>Inverse Disabled</button>
+        <button type="button" class="btn" disabled>Base Disabled</button>


I think the problem with replacing the class in this example is that it is still possible to have an inverse button that is disabled and get the old style. Shouldn't we tweak the inverse disabled button styling?

I also think using the base disabled style here sticks out like a sore thumb compared to the other inverse buttons next to it.

Yes, I agree! The disabled-ness of it draws a lot of attention. @chris-mike -- what do you think? Is there another color combination?

thallada · 2016-11-15T20:45:22Z

pattern-library/sass/patterns/_buttons.scss

-        border-color: palette(primary, accent);
-        background: palette(primary, accent);
+        border-color: $btn-focus-color;
+        background: $btn-focus-color;
        color: palette(primary, x-back);
    }



I know this wasn't part of this PR, but do we have a ticket for improving the disabled button text color? It's way too low of a contrast, I'm sure it would trip the accessibility checker.

Edit: Actually, I just looked into this because I was curious how other people handle low-contrast disabled buttons, and the W3C gives an explicit exception for disabled UI elements: http://ux.stackexchange.com/a/80885 So maybe we don't need to change it...

I thought it difficult to read too and thank you for bringing that up. @chris-mike -- I'm happy yo change the disabled button text color here (should just be a line) or we can address this in a follow up story if more discovery is needed for a recommendation.

dsjen · 2016-11-15T21:57:14Z

@andy-armstrong -- thanks for bringing attention the hover outline. @chris-mike -- do you have a style in mind to address the odd looking outline in the "elevated" button now that we've darkened the hover color?

Also, the inverse disabled button is what was asked for in the ticket, but I agree that it looks very odd...even like an error. @chris-mike -- recommendation on the disabled button with inverse background?

bjacobel · 2016-11-16T14:48:16Z

👍

dsjen · 2016-11-16T16:29:55Z

I've made a few changes that @chris-mike suggested:

added text beneath link heading
adjusted inverse disabled button styles

Please see the screenshot as I'm unable to deploy a sandbox:

Outstanding questions:

Generally, contrast on text for disabled buttons seems low. Should we address this now or on a separate ticket?
The elevated button on hover seem to suggest that it's depressed rather. I did a quick search on edx-platform and see no usage of it. Can we remove it? Do we care if it looks funny at this point?

bjacobel · 2016-11-16T16:41:10Z

+1 for removing the elevated button, I don't think it's consistent with any of our existing branding/style.

andy-armstrong · 2016-11-16T20:29:32Z

+1 for removing the elevated buttons from me too. I don't see any usage of them anywhere.

@talbs can you give us the history on elevated buttons?

dsjen · 2016-11-16T20:46:45Z

In response to our meeting this afternoon, I've updated this PR with the following:

darkened disabled button text
removed elevated buttons entirely

andy-armstrong

👍 This is such a great, and long overdue, clean up. Thanks @dsjen!

I have some minor comments if you want to address them now, but no worries if you choose not to.

andy-armstrong · 2016-11-16T20:52:54Z

pattern-library/sass/patterns/_buttons.scss

@@ -120,8 +120,8 @@ $btn-small-font-size: font-size(small);
    &.is-hovered,
    &:focus,
    &.is-focused {
-        border-color: palette(primary, accent);
-        background: palette(primary, accent);
+        border-color: $btn-focus-color;


Thanks for making these into variables! We should strive never to use palette directly in our Sass, but instead use variables. This allows a theme to override the value for the variables and get a consistent experience.

andy-armstrong · 2016-11-16T20:53:24Z

pattern-library/sass/patterns/_buttons.scss

-        border-color: palette(primary, accent);
-        background: palette(primary, accent);
+        border-color: $btn-focus-color;
+        background: $btn-focus-color;
        color: palette(primary, x-back);


I'd understand if you see this as out of scope, but would you mind making this a variable too?

Sure thing! Updating this to use $btn-focus-text-color.

andy-armstrong · 2016-11-16T20:53:39Z

pattern-library/sass/patterns/_buttons.scss

@@ -138,7 +138,7 @@ $btn-small-font-size: font-size(small);
    &:disabled,
    &.is-disabled {
        border-color: palette(grayscale, back);
-        color: palette(grayscale, back);
+        color: palette(grayscale, accent);


Can you make this a variable too.

andy-armstrong · 2016-11-16T20:54:08Z

pattern-library/sass/patterns/_buttons.scss

-            border-color: palette(grayscale, back);
-            color: palette(grayscale, back);
-        }
+        border-color: palette(grayscale, accent);


Can you make these use variables.

andy-armstrong · 2016-11-16T20:56:18Z

pldoc/_components/buttons.md

 </div>

 <h3 class="example-set-hd">Links without visual styling</h3>
+<p>To indicate 'read' or 'visited' links in content areas, use color Primary Dark.</p>


I find myself confused by this. I read this as advice to the user of the Pattern Library, but this isn't something in their control. Maybe you can make it sound less like an option, such as

Note: unread links use the primary color, while read/visited links use primary dark.

Makes sense! FYI, @chris-mike

dsjen · 2016-11-16T22:09:10Z

Added the following variables:

$btn-focus-color:                           palette(primary, dark) !default;
$btn-focus-text-color:                      palette(primary, x-back) !default;
$btn-disabled-text-color:                   palette(grayscale, accent) !default;
$btn-disabled-background-color:             palette(grayscale, x-back) !default;
$btn-disabled-border-color:                 palette(grayscale, back) !default;

Updated buttons to be more accessible.

00e7df1

* darkened colors * updated inverse disabled butotn * updated colors and read state on links

thallada reviewed Nov 15, 2016

View reviewed changes

adjusted inverse disabled styles, added copy beneath link heading

0bdaf47

made disabled inverse button the same color as others

533c685

removed elevated buttons, darkened disabled button text

4c73e84

andy-armstrong approved these changes Nov 16, 2016

View reviewed changes

added button variables, updated link description text

0ca52ca

dsjen merged commit 406b219 into master Nov 16, 2016

dsjen deleted the dsjen/a11y-buttons branch November 16, 2016 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated buttons to be more accessible. #373

Updated buttons to be more accessible. #373

dsjen commented Nov 15, 2016 •

edited by bjacobel

Loading

mention-bot commented Nov 15, 2016

andy-armstrong commented Nov 15, 2016

thallada left a comment

thallada Nov 15, 2016

dsjen Nov 15, 2016

thallada Nov 15, 2016 •

edited

Loading

dsjen Nov 15, 2016

dsjen commented Nov 15, 2016

bjacobel commented Nov 16, 2016

dsjen commented Nov 16, 2016

bjacobel commented Nov 16, 2016

andy-armstrong commented Nov 16, 2016

dsjen commented Nov 16, 2016

andy-armstrong left a comment

andy-armstrong Nov 16, 2016

andy-armstrong Nov 16, 2016

dsjen Nov 16, 2016 •

edited

Loading

andy-armstrong Nov 16, 2016

andy-armstrong Nov 16, 2016

andy-armstrong Nov 16, 2016

dsjen Nov 16, 2016

dsjen commented Nov 16, 2016

Updated buttons to be more accessible. #373

Updated buttons to be more accessible. #373

Conversation

dsjen commented Nov 15, 2016 • edited by bjacobel Loading

Description

Manual Test Plan

Testing Checklist

Non-testing Checklist

Post-review

Reviewers

mention-bot commented Nov 15, 2016

andy-armstrong commented Nov 15, 2016

thallada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thallada Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsjen commented Nov 15, 2016

bjacobel commented Nov 16, 2016

dsjen commented Nov 16, 2016

bjacobel commented Nov 16, 2016

andy-armstrong commented Nov 16, 2016

dsjen commented Nov 16, 2016

andy-armstrong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsjen Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsjen commented Nov 16, 2016

dsjen commented Nov 15, 2016 •

edited by bjacobel

Loading

thallada Nov 15, 2016 •

edited

Loading

dsjen Nov 16, 2016 •

edited

Loading