Skip to content

Conversation

@davidwren-boxuk
Copy link

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

image

image

Copy link
Contributor

@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
Contributor

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.

3 participants