Skip to content
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

Global Styles: add accessible label to Back button #35325

Merged
merged 3 commits into from
Oct 5, 2021
Merged

Global Styles: add accessible label to Back button #35325

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60055e6
Global Styles: add fallback label to `NavigationButton` when navigati…
ciampo Oct 4, 2021
af9a939
Update copy: from "screen" to "view"
ciampo Oct 4, 2021
a528e22
Add the back button s label directly from `ScreenHeader`
ciampo Oct 5, 2021
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
13 changes: 12 additions & 1 deletion packages/edit-site/src/components/global-styles/navigation-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,29 @@ import {
FlexItem,
__experimentalHStack as HStack,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { Icon } from '@wordpress/icons';

function NavigationButton( {
path,
icon,
children,
label: labelProp,
isBack = false,
...props
} ) {
const navigator = useNavigator();

const defaultLabel = isBack
? __( 'Navigate to the previous screen' )
ciampo marked this conversation as resolved.
Show resolved Hide resolved
: undefined;

return (
<Item onClick={ () => navigator.push( path, { isBack } ) } { ...props }>
<Item
onClick={ () => navigator.push( path, { isBack } ) }
aria-label={ labelProp ?? defaultLabel }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be label prop like the Button component. I also wonder why it's not taking the "content" as a label like it's supposed to do by default for buttons.

Copy link
Contributor Author

@ciampo ciampo Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be label prop like the Button component.

The Item component renders a vanilla HTML button (and not a WordPress Button — sorry if I caused any confusion in a previous comment where I stated the opposite).

I also wonder why it's not taking the "content" as a label like it's supposed to do by default for buttons.

I believe it is, but in the case of the back buttons in the Global Styles sidebar, these buttons only have an icon as their content (no human-readable text)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it Thanks, should we use a label prop to align with Button or keep the raw aria-label? I think it's a bit mixed right now in the different components we have and not sure what's the best path forward (maybe a small preference for label in components where it's a generic use-case for compatibility with mobile).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it would be nice to have a less mixed approach to how we pass and name props related to labels.

Given the fact that Item renders a HTML button, at the moment we have to pass the aria-label prop to Item (since label is not a standard HTML attribute).
Maybe a way to improve the situation would be to rename the label prop on the NavigationButton component to aria-label? I could also remove the changes from the NavigationButton and pass the aria-label attribute directly from the ScreenHeader component:

diff --git a/packages/edit-site/src/components/global-styles/header.js b/packages/edit-site/src/components/global-styles/header.js
index 1ddd562aa2..1496bb440c 100644
--- a/packages/edit-site/src/components/global-styles/header.js
+++ b/packages/edit-site/src/components/global-styles/header.js
@@ -31,6 +31,7 @@ function ScreenHeader( { back, title, description } ) {
 						}
 						size="small"
 						isBack
+						aria-label={ __( 'Navigate to the previous screen' ) }
 					/>
 				</View>
 				<Spacer>

Separately from this PR, we could consider rendering a WordPress Button instead of a HTML button inside Item, which could help to make our APIs a bit more coherent

Copy link
Contributor Author

@ciampo ciampo Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I could also remove the changes from the NavigationButton and pass the aria-label attribute directly from the ScreenHeader component

I went ahead and applied that suggested change in a528e22 (#35325) — which greatly simplifies the code changes in this PR

{ ...props }
>
{ icon && (
<HStack justify="flex-start">
<FlexItem>
Expand Down