-
Notifications
You must be signed in to change notification settings - Fork 931
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
USWDS: style aria-disabled to match disabled #4783
Conversation
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.
Looks good so far! Added some comments below. Show disabled is for aria-disabled
exclusively, right? Do you think it'd be easier to add that attribute on the existing elements vs adding new items?
packages/usa-button-group/src/content/usa-button-group~segmented.json
Outdated
Show resolved
Hide resolved
{% if aria_disabled == true %} | ||
<div class="usa-checkbox"> | ||
<input class="usa-checkbox__input" id="check-historical-carver" type="checkbox" name="historical-figures" value="george-washington-carver" disabled> | ||
<label class="usa-checkbox__label" for="check-historical-carver">George Washington Carver</label> | ||
</div> | ||
{% endif %} |
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.
Thoughts on adding this existing elements if disabled instead?
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 dig it. Updated in de783f3.
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.
Also added this behavior to usa-radio: acd1289
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.
And added to button group: 01dafee
@mejiaj
Please let me know if you have any questions or concerns. |
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.
Sorry, one last item. Do you think the disabled controls might be better if we combined them into radio or select? Because right now you can have both disabled and aria-disabled selected.
The options being something like:
Disabled state:
- None
- Disabled
- Aria-disabled
Good call! I'll fix now. |
@mejia I have updated the controls to use radio buttons wherever both disabled and aria-disabled can be applied. Please let me know if you see anything else. |
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.
Added some suggestions and comments for clarifying button group template. I realize this is outside the scope of the PR, so happy to move it to a separate issue. If you think so too, can you create a new issue for it?
The styling looks good.
<li class="usa-button-group__item"> | ||
{{ item }} | ||
<button class="usa-button" {% if disabled_state == "disabled" %}disabled{% endif %} {% if disabled_state == "aria-disabled" %}aria-disabled="true"{% endif %}>Map</button> |
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.
Maybe an if/elseif statement might be more readable — along with some formatting.
<button class="usa-button" {% if disabled_state == "disabled" %}disabled{% endif %} {% if disabled_state == "aria-disabled" %}aria-disabled="true"{% endif %}>Map</button> | |
<button class="usa-button" | |
{% if disabled_state == "disabled" %} | |
disabled | |
{% elseif disabled_state == "aria-disabled" %} | |
aria-disabled="true" | |
{% endif %} | |
> | |
Map | |
</button> |
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.
Opened issue #4881 to resolve in a future enhancement
{% for modifier in modifiers %} | ||
{% if 'usa-button--outline usa-button--inverse' in modifier %} | ||
<div class="bg-base-darkest padding-1" style="max-width: fit-content"> | ||
{% endif %} | ||
<ul class="usa-button-group usa-button-group--segmented"> | ||
<li class="usa-button-group__item"> | ||
<button class="usa-button {{ modifier }}" {% if disabled_state == "disabled" %}disabled{% endif %} {% if disabled_state == "aria-disabled" %}aria-disabled="true"{% endif %}>Map</button> | ||
</li> | ||
<li class="usa-button-group__item"> | ||
<button class="usa-button {{ modifier }}" {% if disabled_state == "disabled" %}disabled{% endif %} {% if disabled_state == "aria-disabled" %}aria-disabled="true"{% endif %}>Hybrid</button> | ||
</li> | ||
<li class="usa-button-group__item"> | ||
<button class="usa-button {{ modifier }}" {% if disabled_state == "disabled" %}disabled{% endif %} {% if disabled_state == "aria-disabled" %}aria-disabled="true"{% endif %}>Satellite</button> | ||
</li> | ||
</ul> | ||
{% if 'usa-button--outline usa-button--inverse' in modifier %} | ||
</div> | ||
{% endif %} |
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.
If we added items to usa-button-group~segmented.json
we could loop too.
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.
Opened issue #4881 to resolve in a future enhancement
@mejiaj I opened issue #4881 so that we can address these concerns about the button-group template in a future enhancement. |
Description
Closes: #4508
Preview links:
Problem
At times, users of the Design System will want to use
aria-disabled
in place of thedisabled
attribute, but are limited because currently there is no explicit styling foraria-disabled
elements in the Design System.aria-disabled
provides a different experience to end-users that use keyboard navigation or assistive technologies. The primary difference between the two attributes is this:disabled
elements do not receive focus on tab, which can result in the item being:aria-disabled
does receive focus on tab, which can give users important clues about their next stepsThe movement in industry conversation seems to be towards
aria-disabled
as the preferred user experience (See resources below), but other mainstream design systems still use thedisabled
attribute. This item is only one piece of a larger discussion about the UX/accessibility of disabled buttons.From Mozilla:
Resources
Solution
Providing styling for both
aria-disabled
anddisabled
will give users more flexibility to make the appropriate usability decisions for their site.aria-disabled
. It is not intended to replacedisabled
as the default attribute in our components.Tasks completed
:disabled
is declaredauto
instead ofpointer
cursor: not-allowed
. Should we standardize to one value?Testing
:disabled
SCSS references also includearia-disabled
disabled
andaria-disabled
align on the following components:Additional information
Before you hit Submit, make sure you’ve done whichever of these applies to you:
npm test
and make sure the tests for the files you have changed have passed.