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

[FEATURE] Add ARIA controls to block #137

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

davidwren-boxuk
Copy link

Add the ability to set ARIA controls in the icon block sidebar:

image

image

Copy link
Collaborator

@jdamner jdamner left a comment

Choose a reason for hiding this comment

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

Is this is likely to create the warning for block recovery for existing icons? Can we test to see if that warning appears? This block is on use in many sites so it would be better if we can avoid if so. Looking at the code you might be ok if you change the aria-hidden attribute to have a true or undefined value instead of true/false. If it's undefined I think React will omit it from the HTML generated. To avoid the warning we need to be sure the HTML from the save function is unchanged for existing icons.

Also, have we considered the default values here, not sure what the "correct" value is from an accessibility standpoint but we should probably set the default value to be the correct value. We could also default the label to being the text value of the icon, perhaps sentence casing it with icon suffix?

@jdamner
Copy link
Collaborator

jdamner commented Sep 11, 2024

Also, forgot to say though, great addition!! Glad we're looking at things like this.

@davidwren-boxuk
Copy link
Author

I've added this for our project so thought it would be a good candidate to push back upstream.

I think the default is for screen readers to read out any text which would equate to aria-hidden=false which is why I went for that default, but take your point about it breaking existing blocks - so setting it to undefined would make more sense, as you say React should omit the attribute in that case. Will do some further testing!

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.

2 participants