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

rewrite accessibility section on invokers #1078

Merged
merged 4 commits into from
Sep 10, 2024

Conversation

keithamus
Copy link
Collaborator

This rewrites the a11y section on invokers to be far more comprehensive, detailing the exact intentions we have when it comes to applying various ARIA states to the button, as well as focus fixups.

Copy link
Collaborator

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

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

Wow, awesome new section! This looks good to me, but I'm not the right person to judge it. I'll stamp this approved, because I'm ok landing it and putting up PRs to improve it over time. But @scottaohara and @aleventhal please take a look and also @ mention anyone else that should have a look.

@scottaohara
Copy link
Collaborator

i'll review this later today or tomorrow. thanks @keithamus

Copy link
Collaborator

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

let me know if there are questions with my comments

site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
@mfreed7
Copy link
Collaborator

mfreed7 commented Aug 29, 2024

let me know if there are questions with my comments

@keithamus is the plan still to update this with @scottaohara 's comments before landing? (I hope so.)

@scottaohara
Copy link
Collaborator

we are going to need the HTML AAM PR for this as well, so whether fixed here or in that PR (and then this gets updated to just say 'go look at html aam' instead) is fine by me.

but from what i understand, we are going to need that HTML AAM PR to land at the same / close to the same time as the HTML PR. so we need to get that going / address the above either way.

keithamus and others added 2 commits September 9, 2024 10:07
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
@keithamus
Copy link
Collaborator Author

@scottaohara I believe I've addressed your review commentary. If you would kindly give it another pass that would be much appreciated.

Copy link
Collaborator

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

approving with minor additional suggestions. take them as you will. thank you!

site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/invokers.explainer.mdx Outdated Show resolved Hide resolved
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
@keithamus
Copy link
Collaborator Author

Thanks @scottaohara! I'll merge and get to work on the AAM PRs!

@scottaohara
Copy link
Collaborator

Thanks @scottaohara! I'll merge and get to work on the AAM PRs!

excellent. thank you. just give me a holler if you have any questions when making that PR.

@keithamus keithamus merged commit 755329e into main Sep 10, 2024
5 checks passed
@keithamus keithamus deleted the rewrite-accessibility-section-on-invokers branch September 10, 2024 21:18
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.

3 participants