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.

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(Alert): update alert a11y to address title elements #3348

Merged
merged 3 commits into from
Jan 23, 2023

Conversation

nicolethoen
Copy link
Collaborator

@nicolethoen nicolethoen commented Jan 16, 2023

Closes: #3336

Update the alert a11y docs to reflect the changes made in patternfly/patternfly-react#8518

@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 16, 2023

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Couple of verbiage tweaks below. Also it looks like the formatting of the file got tinkered with when saving.

@@ -19,21 +21,27 @@ At a minimum, an alert should meet the following criteria:
<ListItem>
<Checkbox id="alert-a11y-checkbox-1" label="If an alert will dynamically appear or update, it is rendered inside of an alert group component." />
</ListItem>
<ListItem>
<Checkbox id="alert-a11y-checkbox-2" label="If an alert is using a heading level for its title, the heading level should fit the page heading level order so that heading levels are not skipped." />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Checkbox id="alert-a11y-checkbox-2" label="If an alert is using a heading level for its title, the heading level should fit the page heading level order so that heading levels are not skipped." />
<Checkbox id="alert-a11y-checkbox-2" label="If an alert is using a heading element for its title, the heading level fits the page heading level order so that heading levels are not skipped." />

<Checkbox id="alert-a11y-checkbox-2" label="If an alert is using a heading level for its title, the heading level should fit the page heading level order so that heading levels are not skipped." />
</ListItem>
<ListItem>
<Checkbox id="alert-a11y-checkbox-2" label="If an alert's title is not describing any content in a description below it, the title should be wrapped in a non-heading element such as a 'span' or a 'div'." />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Checkbox id="alert-a11y-checkbox-2" label="If an alert's title is not describing any content in a description below it, the title should be wrapped in a non-heading element such as a 'span' or a 'div'." />
<Checkbox id="alert-a11y-checkbox-2" label="If an alert does not have any description below its title, the title is wrapped in a non-heading element such as a 'span' or a 'div'." />

I can't think of a good way to word one at the moment, but a description here quickly summarizing the "why" would be great. Best I can come up with is, "Without a description, a heading element in the title may cause other, unrelated content that follows seem related when navigating the page via rotor menus." Not a blocker if we don't get a description in, though.

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Looks good! It'd be worth reverting the formatting in the "React customization" table just to keep it consistent

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

Looks good! :)

@nicolethoen nicolethoen merged commit 525c86f into patternfly:main Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alert - update a11y docs
4 participants