chore: better a11y for onboarding; #3584
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
| @@ -101,13 +101,15 @@ function ProfilePictureComponent( | |||
| return null; | |||
| } | |||
|
|
|||
| const imageAlt = `${user.username || user.name || user.id}'s profile`; | |||
There was a problem hiding this comment.
Alt for those kind of images was "Undedined's profile picture"
| <h2 {...attrs} className="text-center font-bold typo-title2"> | ||
| {title} | ||
| </h2> |
There was a problem hiding this comment.
Need to pass id for aria-labelledby
| @@ -73,7 +73,7 @@ export const AuthSignBack = ({ | |||
| <ConditionalWrapper | |||
| condition={provider !== 'password'} | |||
| wrapper={(component) => ( | |||
| <div className="relative"> | |||
| <div className="relative" aria-hidden> | |||
There was a problem hiding this comment.
Onboarding's profile image is not a required information on screen reader, maybe is better to skip just to the login button since image cannot be seen in those cases.
| <ArrowIcon | ||
| aria-hidden | ||
| className="rotate-90" | ||
| role="presentation" | ||
| /> |
There was a problem hiding this comment.
This kind of icons that are only presentational and gives no utility to the screen reader user are better to be not present in their navigation
There was a problem hiding this comment.
not urgent, but we should probably add some config to Icon component maybe to make it easier.
| @@ -52,6 +52,7 @@ function LoginForm({ | |||
|
|
|||
| return ( | |||
| <AuthForm | |||
| aria-label="Login using email and password" | |||
There was a problem hiding this comment.
Made this to give a name to the form, this should be mandatory but, as a general optimization, we can also make it point with aria-labelledby to the form title if we are 100% sure to have one
| className={classNames( | ||
| 'flex items-center justify-center text-text-quaternary typo-callout', | ||
| className, | ||
| )} | ||
| role="separator" |
There was a problem hiding this comment.
I don't think that "or" need to be read, but open to discussion
| aria-label={`Continue as ${ | ||
| signBack.name || signBack.email | ||
| } from ${provider}`} |
There was a problem hiding this comment.
trying to give more context to the user
| <a href={`${webappUrl}users`}>Leaderboard</a> | ||
| </li> | ||
| </ul> | ||
| <footer> |
There was a problem hiding this comment.
Those links are always in the end of the page, footer is a better tag element imo
| @@ -846,7 +846,7 @@ describe('Feed', () => { | |||
| const title = await screen.findByTestId('post-modal-title'); | |||
| expect(title).toHaveTextContent(firstPost.node.title); | |||
|
|
|||
| await screen.findByRole('navigation'); | |||
| await screen.findAllByRole('navigation'); | |||
There was a problem hiding this comment.
wasn't working because is finding more than one element that has this role
| export interface IconProps extends ComponentProps<'svg'> { | ||
| secondary?: boolean; | ||
| size?: IconSize; |
There was a problem hiding this comment.
Need to make the icons able to receive aria- attributes
There was a problem hiding this comment.
ComponentProps is prefered anyway :)
| if (firstChild instanceof HTMLElement) { | ||
| firstChild.focus(); | ||
| } | ||
| }, 0); |
There was a problem hiding this comment.
Need to wait for next rendering before moving focus into the new tab element.
if we don't focus the new tab user will loose the focus from the entire page once that the current tab-changing triggering button will disappear.
ilasw
changed the title
…lydotdev/apps into fix-onboarding-accessibility
…nboarding-accessibility
…to fix-onboarding-accessibility
Changes
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Preview domain
https://fix-onboarding-accessibility.preview.app.daily.dev