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

a11y: Add aria-label attributes and custom override control to core/social-link block #18651

Merged
merged 11 commits into from Dec 27, 2019

Conversation

@0aveRyan
Copy link
Member

0aveRyan commented Nov 21, 2019

Description

Fixes #18620.

Per #18620, I've modified the core/social-link block to contain an aria-label attribute for the anchor elements wrapping the social icon SVGs.

Deque classifies missing descriptive text for links as a serious user impact and Google Lighthouse accessibility scores are negatively impacted without an aria-label if the anchor doesn't contain text or an image with an alt description.

By default, if the user doesn't customize the label in the provided TextControl in the Inspector, the block will render Link to {BRAND}. However, if the block is used 10 times in a document, hearing the same label 10 times isn't very useful if each social icon links to a different profile/type of page, hence the custom attribute to enable clearer descriptions.

At this point, there is a fair amount of duplication between the PHP and JS file containing data about the brands. These are unlikely to change greatly, but I'm happy to address with a different approach if desired.

How has this been tested?

I've run the lint command, inserted each social icon and validated the display and markup for the Editor and Frontend render.

Types of changes

  • Introduces label attribute for the core/social-link block (only stores customizations, default is handled in render method)
  • Introduces no breaking changes and progressively enhances any existing social blocks in-use.

Screenshot

Screenshot of editor displaying new TextControl component for setting aria-label in core/social-block

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
Copy link
Member

mkaz left a comment

This is a nice addition, thanks.
I added a couple minor suggestions.

packages/block-library/src/social-link/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/social-link/index.php Outdated Show resolved Hide resolved
@mkaz

This comment has been minimized.

Copy link
Member

mkaz commented Nov 23, 2019

Unrelated to your changes, I'm still not thrilled with the implementation. I agree there is too much duplication between PHP and JS. The core issue at hand is wrangling the SVG, I have one unsuccessful attempt here: #18243

Additionally, I think we can simplify the list if we can solve the above, and then implement the ability to extend or modify, related issue #17277

@0aveRyan

This comment has been minimized.

Copy link
Member Author

0aveRyan commented Nov 24, 2019

@mkaz Thanks for the review! I'll push a commit to address the description & LinkedIn.

I'm also a fan of svgr, but from the chatter on #18243 sounds like that may require some creative solutions for the Gutenberg codebase.

I agree the implementation is fairly easy to understand, but perhaps leaves something to be desired from a developer experience and maintenance standpoint. Unfortunately, most other approaches I can think of would either transfer that burden to visitors or obfuscate to developers what's happening.

On my brain apart from svgr...

  • Leave implementation as-is, add filter hooks in JS & PHP for extension and enhance with clearer inline documentation that any changes must happen to both objects -- still duplicated but extensible and clearer.
  • Add a socialData.json file with icons base64 encoded and filter hooks in JS & PHP to allow extension -- adds filesystem read in PHP and base64 strings would be about 1/3 larger.
  • Use a socialData.json file to inject the objects into JS & PHP during webpack build, base64 decode icons and add filter hooks -- obfuscates what's happening and some complexity, but reduces client burden.
  • Separate icons into a SVG sprite approach or one-off SVG files, use a socialData.json to point to the proper icon and add filter hooks -- makes a larger payload to transmit to client/increases client requests, but makes using a data file easier.
@mkaz

This comment has been minimized.

Copy link
Member

mkaz commented Nov 25, 2019

One more, Github --> GitHub, see #18714

With regards to SVG, thanks for the suggestions, I'll need to noodle on them a bit more, I like how you draw out the pros and cons to the various solutions. We discuss further in a separate PR, I have on my list to move them out of PHP into single SVG files this would improve one aspect, but need to consider how it might change after this PR, it may not make it easier overall

@0aveRyan

This comment has been minimized.

Copy link
Member Author

0aveRyan commented Dec 3, 2019

@mkaz I've addressed the branding label issues and field helper text. Should we start a new issue to discuss and address the social SVG storage and label data?

@mkaz
mkaz approved these changes Dec 9, 2019
Copy link
Member

mkaz left a comment

Updates looks good, thanks! 👍

Yes, I think a new issue to discuss the alternate storage would be best.

0aveRyan added 4 commits Dec 26, 2019
@0aveRyan

This comment has been minimized.

Copy link
Member Author

0aveRyan commented Dec 26, 2019

@youknowriad I've addressed your feedback, this should be good to go.

0aveRyan added 2 commits Dec 26, 2019
@@ -6,18 +6,23 @@ import classNames from 'classnames';
/**

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 26, 2019

Contributor

I see a few strings here where we use Title Case. I think we've been recently moving to always using "Sentence case" in all of our strings. @karmatosed do you confirm?

This comment has been minimized.

Copy link
@0aveRyan

0aveRyan Dec 26, 2019

Author Member

Screen Shot 2019-12-26 at 10 03 00 AM

@youknowriad that doesn't appear to be the standard used for Panel Titles and that doesn't seem to be the standard outlined in the docs. Was there a discussion about that I missed? If that's changed, the docs need to be updated and issues opened to address the many instances of title case throughout the application.

This comment has been minimized.

Copy link
@0aveRyan

0aveRyan Dec 27, 2019

Author Member

I've since dug and found the PRs and issue discussion around sentence case. A major departure like that should be discussed and memorialized in docs with clearer examples well before it's put into practice, to set clear expectations, avoid confusing situations like this and throwing up hurdles to contribution. Let me know if you'd like these strings updated to sentence case.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 27, 2019

Contributor

I don't know to be honest, I'll let @karmatosed chime in when she comes back. I think we can land this as is for now.

@youknowriad youknowriad merged commit 2cdeb19 into WordPress:master Dec 27, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@0aveRyan 0aveRyan deleted the 0aveRyan:a11y/social-link-anchor-labels branch Dec 31, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.