Skip to content

Update buttons to be more accessible #1292

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.

Sign up for GitHub

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

Merged
just-be-dev merged 3 commits into from
Nov 8, 2022
Merged

Update buttons to be more accessible #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8137e50
Add visually disabled state to the button component
just-be-dev Nov 8, 2022
86d3e94
Update tests to match button's new visually disabled state
just-be-dev Nov 8, 2022
bba327a
Update libs/ui/lib/button/Button.tsx
just-be-dev Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/components/form/fields/DateTimeRangePicker.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ describe('custom mode', () => {

expect(onChange).not.toBeCalled()
expect(screen.getByLabelText('Start time')).toBeEnabled()
expect(screen.getByRole('button', { name: 'Reset' })).toBeDisabled()
expect(screen.getByRole('button', { name: 'Load' })).toBeDisabled()
expect(screen.getByRole('button', { name: 'Reset' })).toHaveClass('visually-disabled')
expect(screen.getByRole('button', { name: 'Load' })).toHaveClass('visually-disabled')
})

it('clicking load after changing date changes range', async () => {
Expand Down
32 changes: 30 additions & 2 deletions libs/ui/lib/button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import cn from 'classnames'
import type { MouseEventHandler } from 'react'
import { forwardRef } from 'react'

import { Spinner } from '@oxide/ui'
Expand Down Expand Up @@ -90,18 +91,45 @@ export const buttonStyle = ({
)
}

/**
* When the `disabled` prop is passed to the button we put it in a visually disabled state.
* In that case we want to override the default mouse down and click behavior to simulate a
* disabled state.
*/
const noop: MouseEventHandler<HTMLButtonElement> = (e) => {
e.stopPropagation()
e.preventDefault()
}

Copy link
Collaborator

@david-crespo david-crespo Nov 8, 2022

Choose a reason for hiding this comment

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

Love this. Hope we don't find this breaks some other click behavior in the container.

// Use `forwardRef` so the ref points to the DOM element (not the React Component)
// so it can be focused using the DOM API (eg. this.buttonRef.current.focus())
export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
(
{ children, size, variant, color, className, loading, innerClassName, ...rest },
{
children,
size,
variant,
color,
className,
loading,
innerClassName,
disabled,
onClick,
'aria-disabled': ariaDisabled,
...rest
},
ref
) => {
return (
<button
className={cn(buttonStyle({ size, variant, color }), className)}
className={cn(buttonStyle({ size, variant, color }), className, {
'visually-disabled': disabled,
})}
ref={ref}
type="button"
onMouseDown={disabled ? noop : undefined}
onClick={disabled ? noop : onClick}
aria-disabled={disabled || ariaDisabled}
{...rest}
>
<>
Expand Down
12 changes: 8 additions & 4 deletions libs/ui/lib/button/button.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,33 @@
.btn-primary {
@apply text-accent bg-accent-secondary hover:bg-accent-secondary-hover disabled:text-accent-disabled disabled:bg-accent-secondary;
}
.btn-primary:disabled > .spinner {
.btn-primary:disabled > .spinner,
.btn-primary.visually-disabled > .spinner {
@apply text-accent;
}

/* Using `-solid` since ghost version is used as default secondary button state */
.btn-secondary-solid {
@apply text-secondary bg-secondary hover:bg-secondary-hover disabled:text-quaternary disabled:bg-secondary;
}
.btn-secondary-solid:disabled > .spinner {
.btn-secondary-solid:disabled > .spinner,
.btn-secondary-solid.visually-disabled > .spinner {
@apply text-secondary;
}

.btn-destructive {
@apply text-destructive bg-destructive-secondary hover:bg-destructive-secondary-hover disabled:text-destructive-disabled disabled:bg-destructive-secondary;
}
.btn-destructive:disabled > .spinner {
.btn-destructive:disabled > .spinner,
.btn-destructive.visually-disabled > .spinner {
@apply text-destructive;
}

.btn-notice {
@apply text-notice bg-notice-secondary hover:bg-notice-secondary-hover disabled:text-notice-disabled disabled:bg-notice-secondary;
}
.btn-notice:disabled > .spinner {
.btn-notice:disabled > .spinner,
.btn-notice.visually-disabled > .spinner {
@apply text-notice;
}

Expand Down
1 change: 1 addition & 0 deletions tailwind.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ module.exports = {
addVariant('children', '& > *')
addVariant('between', '& > * + *')
addVariant('selected', '.is-selected &')
addVariant('disabled', '&.visually-disabled, &:disabled')
Copy link
Collaborator

@david-crespo david-crespo Nov 8, 2022

Choose a reason for hiding this comment

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

🔥

addUtilities(
Array.from({ length: 12 }, (_, i) => i)
.map((i) => ({
Expand Down