-
Notifications
You must be signed in to change notification settings - Fork 367
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
fix: [M3-7571] - Missing label for Full Account Access Toggle #10931
Conversation
Coverage Report: ✅ |
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.
Thx! - Accessibility fix looks good but the headings composition needs to be reverted
packages/manager/src/features/Users/UserPermissionsEntitySection.tsx
Outdated
Show resolved
Hide resolved
@@ -371,7 +345,7 @@ class UserPermissions extends React.Component<CombinedProps, State> { | |||
spacing={2} | |||
> | |||
<Grid> | |||
<Typography data-qa-permissions-header="billing" variant="h3"> | |||
<Typography data-qa-permissions-header="billing" variant="h2"> |
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.
same - the headings composition was already correct
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.
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.
Yea these are good pending changes
cy.findByLabelText('Toggle Full Account Access') | ||
.should('be.visible') | ||
.click(); | ||
cy.findByText('Full Account Access').should('be.visible').click(); |
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.
This might be a dumb question, but wouldn't the fact that we switched to findByText
over findByLabelText
indicate that it isn't accessible? Is it expected we can't use findByLabelText
?
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.
@bnussman-akamai It's a valid question and I was confused by this as well. The error from running the cypress debug command states "The element is not visible because it has CSS property: opacity: 0
."
Looking at the Cypress docs, as a best practice, they recommend using data-*
attributes to provide context to selectors and isolate them from CSS or JS changes. With that in mind, I went ahead and implemented this and confirmed the tests are still passing and VoiceOver works as expected.
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.
Understood! Thanks for the context 🙏
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.
Thanks for addressing the changes - change looks good from my end and addresses the accessibility issue ✅
Description 📝
This PR addresses the issue of the "Full Account Access" toggle missing a proper label. The changes in this PR improve accessibility in two ways. First by implementing correct markup semantics. Second, by ensuring screen readers inform the user more adequately by enhancing the output that is read.
Changes 🔄
List any change relevant to the reviewer.
variant
property for someTypography
components to ensure properHeader
levels.Target release date 🗓️
09/30/2024
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply