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

Components: Adds AccessibleSVG component and use consistently for block icons #9565

Merged
merged 6 commits into from Sep 4, 2018

Conversation

@youknowriad
Contributor

youknowriad commented Sep 3, 2018

closes #9372

  • This PR adds an AccessibleSVG component
  • Use this component in BlockIcon

Test that the svg icons contain the required A11y props.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 3, 2018

Contributor

This is impressive. From my quick testing, it seems like all the block icons had the proper attributes as they need. Fine work! I can't really speak too much for the code — it looks good to me. But from everything else, 👍 👍

Thanks for working on this.

Contributor

jasmussen commented Sep 3, 2018

This is impressive. From my quick testing, it seems like all the block icons had the proper attributes as they need. Fine work! I can't really speak too much for the code — it looks good to me. But from everything else, 👍 👍

Thanks for working on this.

@tofumatt

I think some documentation around what happens here would be useful as right now this is elegant, but a bit Magic™.

Overall a cool solution though, and definitely one that prevents third-party developers from making mistakes.

I trust you to add some kind of documentation or something, so 👍 with docs. Feel free to ping for further review if needed 😄

@@ -18,6 +18,8 @@ function renderIcon( icon ) {
}
return icon();
} else if ( icon && icon.type === 'svg' ) {

This comment has been minimized.

@tofumatt

tofumatt Sep 3, 2018

Member

That is, I suppose, a better solution that prevents developers who don't bother to use our components from causing an error. It's definitely less explicit though and I don't see any documentation around it: either that using AccessibleSVG directly is preferred or even that if the icon prop is an SVG that we'll add properties to it. We should probably at least document this, as it might be quite jarring to a block developer to see their icon markup change unexpectedly like this with no documentation.

@tofumatt

tofumatt Sep 3, 2018

Member

That is, I suppose, a better solution that prevents developers who don't bother to use our components from causing an error. It's definitely less explicit though and I don't see any documentation around it: either that using AccessibleSVG directly is preferred or even that if the icon prop is an SVG that we'll add properties to it. We should probably at least document this, as it might be quite jarring to a block developer to see their icon markup change unexpectedly like this with no documentation.

This comment has been minimized.

@youknowriad

youknowriad Sep 3, 2018

Contributor

I personally don't think it's important enough for people to know precisely that we're adding some props to their SVGs to make them accessible. I guess it doesn't harm to add a mention in the documention but if we do so, we should avoid using AccessibleSVG directly in the components because the behavior is documented in that case.

@youknowriad

youknowriad Sep 3, 2018

Contributor

I personally don't think it's important enough for people to know precisely that we're adding some props to their SVGs to make them accessible. I guess it doesn't harm to add a mention in the documention but if we do so, we should avoid using AccessibleSVG directly in the components because the behavior is documented in that case.

This comment has been minimized.

@tofumatt

tofumatt Sep 3, 2018

Member

That's fair. I think I'd prefer the documentation because it teaches people about an accessibility issue and it means our code can be: 1. a bit leaner (no imports) 2. more idiomatic (regular SVGs)

So maybe just a note of "we add accessibility properties to SVGs" in the block property documentation with a link to the original issue for context or something.

@tofumatt

tofumatt Sep 3, 2018

Member

That's fair. I think I'd prefer the documentation because it teaches people about an accessibility issue and it means our code can be: 1. a bit leaner (no imports) 2. more idiomatic (regular SVGs)

So maybe just a note of "we add accessibility properties to SVGs" in the block property documentation with a link to the original issue for context or something.

@@ -18,7 +18,7 @@ export const settings = {
description: __( 'This block is deprecated. Please use the Paragraph block instead.' ),
icon: <svg role="img" aria-hidden="true" focusable="false" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M7.1 6l-.5 3h4.5L9.4 19h3l1.8-10h4.5l.5-3H7.1z" /></svg>,
icon: <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M7.1 6l-.5 3h4.5L9.4 19h3l1.8-10h4.5l.5-3H7.1z" /></svg>,

This comment has been minimized.

@tofumatt

tofumatt Sep 3, 2018

Member

I get that we don't have to use our component directly, but I wonder why we wouldn't. This is less explicit and we're already changing all of the lines 🤷‍♂️

@tofumatt

tofumatt Sep 3, 2018

Member

I get that we don't have to use our component directly, but I wonder why we wouldn't. This is less explicit and we're already changing all of the lines 🤷‍♂️

This comment has been minimized.

@youknowriad

youknowriad Sep 3, 2018

Contributor

I don't mind, I was hesitant as well.

@youknowriad

youknowriad Sep 3, 2018

Contributor

I don't mind, I was hesitant as well.

This comment has been minimized.

@tofumatt

tofumatt Sep 3, 2018

Member

As mentioned though, feel free to leave them SVG if you add docs. I think that's probably the better approach, upon reflection.

@tofumatt

tofumatt Sep 3, 2018

Member

As mentioned though, feel free to leave them SVG if you add docs. I think that's probably the better approach, upon reflection.

@tofumatt tofumatt added this to the 3.8 milestone Sep 3, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

@tofumatt I added a note to the docs, feel free to tweak it (I'm not great at documentation :P)

Contributor

youknowriad commented Sep 4, 2018

@tofumatt I added a note to the docs, feel free to tweak it (I'm not great at documentation :P)

tofumatt added some commits Sep 4, 2018

@tofumatt

Docs look good; I made a few tweaks (mainly changing a11y to accessibility).

:shipit:

@youknowriad youknowriad merged commit 3b6c167 into master Sep 4, 2018

2 checks passed

codecov/project 50.37% (+<.01%) compared to cf74b67
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/accessible-svg branch Sep 4, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 4, 2018

Contributor

Test that the svg icons contain the required A11y props.

❤️

Contributor

afercia commented Sep 4, 2018

Test that the svg icons contain the required A11y props.

❤️

@caxco93

This comment has been minimized.

Show comment
Hide comment
@caxco93

caxco93 Sep 6, 2018

Contributor

I think we should extract the SVG icons to another package, maybe @wordpress/icons. This will increase their reusability and maintainability. Right now I was looking at #9642 and it would require to declare the same SVG again for the MediaPlaceholder icon.

Here's the PR where I extract the SVG icons to a new package

Then we could simply do it like this
image

@youknowriad @tofumatt

Contributor

caxco93 commented Sep 6, 2018

I think we should extract the SVG icons to another package, maybe @wordpress/icons. This will increase their reusability and maintainability. Right now I was looking at #9642 and it would require to declare the same SVG again for the MediaPlaceholder icon.

Here's the PR where I extract the SVG icons to a new package

Then we could simply do it like this
image

@youknowriad @tofumatt

@jasmussen

This comment has been minimized.

Show comment
Hide comment
Contributor

jasmussen commented Sep 6, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 6, 2018

Contributor

@youknowriad coming a bit late to this, but I feel this should be just called Svg (with capital) as being accessible should be core to all the components we offer.

Contributor

mtias commented Sep 6, 2018

@youknowriad coming a bit late to this, but I feel this should be just called Svg (with capital) as being accessible should be core to all the components we offer.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 6, 2018

Member

That's kinda confusing if you quickly scan it 🤷‍♂️ (<Svg /> vs <svg />), but also the point is the stock SVGs aren't always accessible and the aim of the wrapper is to make them so.

Member

tofumatt commented Sep 6, 2018

That's kinda confusing if you quickly scan it 🤷‍♂️ (<Svg /> vs <svg />), but also the point is the stock SVGs aren't always accessible and the aim of the wrapper is to make them so.

@jasmussen jasmussen referenced this pull request Sep 7, 2018

Open

React Native - SVG icons for Inserter #9294

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment