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

Update Dashicons. #1012

Merged
merged 2 commits into from
Jun 6, 2017
Merged

Update Dashicons. #1012

merged 2 commits into from
Jun 6, 2017

Conversation

jasmussen
Copy link
Contributor

This removes the title element from the sprite. This should finally fix #528.

Corresponding upstream PR: WordPress/dashicons#192

This removes the title element from the sprite. This should finally fix #528.
@jasmussen jasmussen self-assigned this Jun 3, 2017
@jasmussen jasmussen requested a review from afercia June 3, 2017 10:28
@jasmussen
Copy link
Contributor Author

It looks like the tests fail for the icon unit test. Is that test written to look for the title? @BE-Webdesign I think you may have done some of the magic here, can you provide me with a sanity check (not urgent)? I know I made a mistake upstream that I'm fixing, but I'm not sure what's up here. Thank you.

@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented Jun 3, 2017

@jasmussen,

Okay I read over #528, are we supposed to add an aria-label instead? The tests indeed check for the title for accessibility, so they are failing now due to the missing title tag. If you need help let me know I can either fix it for you or show you how to change the tests.

@jasmussen
Copy link
Contributor Author

Okay I read over #528, are we supposed to add an aria-label instead? T

My reading is that whatever label we add should be in a parent containing element:

<button aria-label="Some label here"><SVG here></button>

CC: @afercia

@afercia
Copy link
Contributor

afercia commented Jun 5, 2017

Yes when the icon is not purely decorative and represents content that needs to be announced, then this content can be provided as aria-label on the containing element or, as mentioned in #528, using the Twenty Seventeen approach with screen-reader-text e.g.:

<button type="submit" class="search-submit">
	<svg class="icon icon-search" aria-hidden="true" role="img">
		<use href="#icon-search" xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-search"></use>
	</svg>
	<span class="screen-reader-text">Search</span>
</button>

Either way, the title should be removed.

I forgot to clarify that SVGs should also have aria-hidden="true" role="img", sorry for being unclear.

@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented Jun 5, 2017

My reading is that whatever label we add should be in a parent containing element: <button aria-label="Some label here"><SVG here></button>

You have much better eyesight than me 😲

@BE-Webdesign
Copy link
Contributor

If you need any assistance with the tests let me know, happy to help.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Fixed failing test in 9b21b6f. Up to you if you'd like to merge as-is and address @afercia's comment in a subsequent pull request:

I forgot to clarify that SVGs should also have aria-hidden="true" role="img", sorry for being unclear.

@jasmussen
Copy link
Contributor Author

Thanks! Merging this and putting in an upstream fix again.

@jasmussen jasmussen merged commit 4f28f5e into master Jun 6, 2017
@jasmussen jasmussen deleted the update/dashicons branch June 6, 2017 07:08
jasmussen added a commit to WordPress/dashicons that referenced this pull request Jun 6, 2017
This addresses an accessibility issue with the react component, by adding attributes aria-hidden="true" and role="img" to the component.

See also discussion in WordPress/gutenberg#1012 (review).
jasmussen added a commit that referenced this pull request Jun 6, 2017
This PR adds aria-hidden="true" and role="img" to the SVG files inside the dashicons component, addressing feedback here: #1012 (comment)

Upstream PR: WordPress/dashicons#194

CC: @afercia
@aduth aduth mentioned this pull request Jun 6, 2017
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.

SVG icons accessibility
4 participants