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

fix(homepage): a11y errors #1483

Merged
merged 17 commits into from
Mar 8, 2022
Merged

fix(homepage): a11y errors #1483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9a73792
fix (homepage): a11y empty buttons
0618 Mar 3, 2022
d6eeaf1
fix(docs homepage): a11y for install copy
0618 Mar 4, 2022
73dc138
fix(homepage): a11y issue of the empty label from install code
0618 Mar 4, 2022
e9a2636
fix (homepage): a11y for rating icons
0618 Mar 4, 2022
1d2b644
fix some type error
0618 Mar 4, 2022
2ee9a60
fix(homepage): a11y ARIA reference
0618 Mar 4, 2022
212de8a
fix(homepage): a11y langguage missing
0618 Mar 4, 2022
365f6eb
fix(homepage): a11y fieldset description
0618 Mar 4, 2022
2c07563
update changeset
0618 Mar 5, 2022
b1222e5
Update docs/src/pages/HomePrimitivePreview.tsx
0618 Mar 8, 2022
1a16af5
resolve review comments
0618 Mar 8, 2022
ca834d4
Merge branch 'fix-a11y-home' of github.com:aws-amplify/amplify-ui int…
0618 Mar 8, 2022
94bb127
fix TextField a11y error when lablelHidden
0618 Mar 8, 2022
b948437
add ariaLabel to MenuButton
0618 Mar 8, 2022
20f84aa
refactor: use ComponentClassNames.VisuallyHidden var to replace ampli…
0618 Mar 8, 2022
dde5608
convert primitive related changes. Will do in another PR
0618 Mar 8, 2022
7a40db3
remove unused import
0618 Mar 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
8 changes: 8 additions & 0 deletions .changeset/quiet-impalas-chew.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@aws-amplify/ui-react': patch
Copy link
Contributor

Choose a reason for hiding this comment

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

with the new attributes being added and the structure of some of the components being updated do we want this to be a minor bump instead of a patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point...

I checked the semver doc

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Since we don't have any new features in this PR, I think it's not a MUST.

'@aws-amplify/ui': patch
---

- fix a11y erros on docs homepage
- fix type errors
- rename css class
9 changes: 7 additions & 2 deletions docs/src/components/Copy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { CopyToClipboard } from 'react-copy-to-clipboard';

import { Button } from '@aws-amplify/ui-react';

export const Copy = ({ text, size, variation }) => {
export const Copy = ({ text, size, variation, className }) => {
const [copied, setCopied] = React.useState(false);

const copy = () => {
Expand All @@ -15,7 +15,12 @@ export const Copy = ({ text, size, variation }) => {

return (
<CopyToClipboard text={text} onCopy={copy}>
<Button size={size} variation={variation} isLoading={copied}>
<Button
size={size}
variation={variation}
isLoading={copied}
className={className}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add className because of the modification on docs/src/pages/index.page.tsx

>
{copied ? 'Copied!' : 'Copy'}
</Button>
</CopyToClipboard>
Expand Down
1 change: 1 addition & 0 deletions docs/src/components/Layout/Header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export const Header = ({ platform, colorMode, setColorMode }) => {
<Button
className="docs-header-menu-button"
onClick={() => setExpanded(!expanded)}
ariaLabel="Docs header menu button"
>
{expanded ? <IconClose /> : <IconMenu />}
</Button>
Expand Down
10 changes: 5 additions & 5 deletions docs/src/pages/HomePrimitivePreview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,21 +57,21 @@ export const HomePrimitivePreview = () => {
isExclusive
onChange={(value: string) => setExclusiveValue(value)}
>
<ToggleButton value="align-left">
<ToggleButton value="align-left" ariaLabel="align-left">
<IconFormatAlignLeft />
</ToggleButton>
<ToggleButton value="align-center">
<ToggleButton value="align-center" ariaLabel="align-center">
<IconFormatAlignCenter />
</ToggleButton>
<ToggleButton value="align-right">
<ToggleButton value="align-right" ariaLabel="align-right">
<IconFormatAlignRight />
</ToggleButton>
<ToggleButton value="align-justify">
<ToggleButton value="align-justify" ariaLabel="align-justify">
<IconFormatAlignJustify />
</ToggleButton>
</ToggleButtonGroup>
<Button variation="primary">Get started</Button>
<Menu>
<Menu ariaLabel="homepage demo dropdown menu">
<MenuItem onClick={() => alert('Download')}>Download</MenuItem>
<MenuItem onClick={() => alert('Create a Copy')}>
Create a Copy
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/_document.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const getCSPContent = (context: Readonly<HtmlProps>) => {
class MyDocument extends Document {
render() {
return (
<Html>
<Html lang="en-us">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we may eventually use variables for lang

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a ticket for the docs for a language switcher if it doesn't already exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ticket cut: https://app.asana.com/0/1201840995372760/1201937080448144/f

<Head>
<meta
httpEquiv="Content-Security-Policy"
Expand Down
26 changes: 14 additions & 12 deletions docs/src/pages/index.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { HomePrimitivePreview } from './HomePrimitivePreview';
import { Copy } from '@/components/Copy';
import { Footer } from '@/components/Layout/Footer';

import type { SandpackThemeProp } from '@codesandbox/sandpack-react';
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, we are going to remove sandpack in favor of react-live. #1395 @dbanksdesign

Copy link
Contributor

Choose a reason for hiding this comment

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

potentially... there are some potential security issues with it since it uses eval, still up for debate


const code = `import { AmplifyProvider, Button, Card, Text, Heading, Flex, Badge, Image, StepperField, useTheme } from '@aws-amplify/ui-react';
import '@aws-amplify/ui-react/styles.css';

Expand Down Expand Up @@ -100,7 +102,7 @@ const HomePage = ({ colorMode, setThemeOverride, themeOverride }) => {
const router = useRouter();
const { tokens } = useTheme();
const framework = (router.query.platform as string) ?? 'react';
const sandPackTheme = {
const sandPackTheme: SandpackThemeProp = {
palette: {
activeText: `${tokens.colors.font.interactive}`,
defaultText: `${tokens.colors.font.secondary}`,
Expand Down Expand Up @@ -133,7 +135,6 @@ const HomePage = ({ colorMode, setThemeOverride, themeOverride }) => {
monoFont:
'"Fira Mono", "DejaVu Sans Mono", Menlo, Consolas, "Liberation Mono", Monaco, "Lucida Console", monospace',
fontSize: '16px',
lineHeight: '1.5',
},
};
const installScripts = {
Expand Down Expand Up @@ -170,16 +171,17 @@ const HomePage = ({ colorMode, setThemeOverride, themeOverride }) => {
Get started
<IconChevronRight />
</Button>
<TextField
label=""
labelHidden={true}
isReadOnly={true}
className="install-code"
outerEndComponent={
<Copy variation="link" text={frameworkInstallScript} />
}
value={frameworkInstallScript}
/>
<code className="install-code__container">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<code> tag might be more proper here

<p className="install-code__content">
{frameworkInstallScript}
</p>
<Copy
className="install-code__button"
size=""
variation="link"
text={frameworkInstallScript}
/>
</code>
</Flex>
</Card>
<Flex
Expand Down
14 changes: 12 additions & 2 deletions docs/src/styles/docs/home.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,19 @@
padding: var(--amplify-space-xxl);
}

.install-code {
.install-code__container {
flex-grow: 1;
margin: 0;
display: flex;
}

.install-code__content {
margin: auto;
overflow-x: auto;
}

.install-code__button {
place-self: end;
align-self: center;
}

.docs-home-amp-product-card {
Expand Down
3 changes: 2 additions & 1 deletion packages/react/src/primitives/Menu/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const MenuPrimitive: Primitive<MenuProps, 'div'> = (
size,
trigger,
triggerClassName,
ariaLabel,
onOpenChange,
...rest
},
Expand All @@ -33,7 +34,7 @@ const MenuPrimitive: Primitive<MenuProps, 'div'> = (
<DropdownMenuTrigger asChild={true}>
{trigger ?? (
<MenuButton
role="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why we need to add it for a button, so removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Did it cause an accessibility error? I vaguely remember that without role="button" I couldn't use my own button with the Radix UI Dropdown Menu. Can you make sure removing this didn't break the Menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it doesn't cause any a11y error because it is a button. It would cause error if it's a <div> or something but uses as a button

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested it and the menu button still works correctly. Proceed 👍

ariaLabel={ariaLabel}
size={size}
testId={MENU_TRIGGER_TEST_ID}
className={classNames(
Expand Down
8 changes: 3 additions & 5 deletions packages/react/src/primitives/Rating/RatingIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@ export const RatingIcon: React.FC<RatingIconProps> = ({
className,
}) => {
return (
<View as="span" className={classNames(`amplify-rating-icon-container`)}>
<View as="label" className={classNames(`amplify-rating-label`)}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what's the label for, so removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not sure why there's a label here either. @jacoblogan Do you remember why we used a label here? We should take another look at the accessibility of this component. If we test this in VoiceOver does it read out the visually hidden text? Is there potentially a better way to label a rating component that we haven't seen yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the label tag was a remnant of earlier designs when this component was not read only. Looking at some other rating components out there they use a label and hidden checkbox to mark user selections. @reesscot brought up a good point that we could have customers who are targeting this label for styling and so if we don't need to remove it, then it would be best to leave it in. @0618 other than the extra clutter, was the label causing any problems? If not then I agree that we should leave it in as to not create a potentially breaking change for customers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacoblogan yes, "empty label" is a type of a11y error, so I removed it but kept the class selector in this PR. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. If it's causing a11y error let's remove it.

<View as="span" className={classNames(className)} color={fill}>
{icon}
</View>
<View as="span" className="amplify-rating-icon-container">
<View as="span" className={classNames(className)} color={fill}>
{icon}
</View>
</View>
);
Expand Down
18 changes: 7 additions & 11 deletions packages/react/src/primitives/Rating/RatingMixedIcon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,25 @@ export const RatingMixedIcon: React.FC<RatingMixedIconProps> = ({
}) => {
const widthPercentage = `${(value % 1) * 100}%`;
return (
<View as="span" className={classNames(`amplify-rating-icon-container`)}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why we need classNames here, so removed it

<View as="label" className={classNames(`amplify-rating-label`)}>
<View as="span" className="amplify-rating-icon-container">
<View as="span" className="amplify-rating-label">
<View
as="span"
className={classNames(
`amplify-rating-icon`,
`amplify-rating-icon-empty`
'amplify-rating-icon',
'amplify-rating-icon-empty'
)}
color={emptyColor}
>
{emptyIcon}
</View>
</View>
<View
as="label"
className={classNames(`amplify-rating-label`)}
width={widthPercentage}
>
<View as="span" className="amplify-rating-label" width={widthPercentage}>
<View
as="span"
className={classNames(
`amplify-rating-icon`,
`amplify-rating-icon-filled`
'amplify-rating-icon',
'amplify-rating-icon-filled'
)}
color={fillColor}
>
Expand Down
10 changes: 5 additions & 5 deletions packages/ui/src/theme/css/component/card.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@
border-style: var(--amplify-components-card-border-style);
border-color: var(--amplify-components-card-border-color);
box-shadow: var(--amplify-components-card-box-shadow);
display: inline-block;
display: block;
padding: var(--amplify-components-card-padding);
&[data-variation="outlined"] {

&[data-variation='outlined'] {
background-color: var(--amplify-components-card-outlined-background-color);
border-radius: var(--amplify-components-card-outlined-border-radius);
border-width: var(--amplify-components-card-outlined-border-width);
border-style: var(--amplify-components-card-outlined-border-style);
border-color: var(--amplify-components-card-outlined-border-color);
box-shadow: var(--amplify-components-card-outlined-box-shadow);
}
&[data-variation="elevated"] {

&[data-variation='elevated'] {
background-color: var(--amplify-components-card-elevated-background-color);
border-radius: var(--amplify-components-card-elevated-border-radius);
border-width: var(--amplify-components-card-elevated-border-width);
Expand Down