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

Social Icons: Add the ability to show/hide labels #38152

Merged
merged 6 commits into from Jan 27, 2022

Conversation

ndiego
Copy link
Member

@ndiego ndiego commented Jan 21, 2022

Description

The Social Links (Icons) block allows you add custom link labels. Unfortunately you are not able to display these text labels, which has been requested in #31605. This PR fixes #31605 by adding the ability to toggle on and off link labels. When enabled, service name is shown if no custom label has been specified.

How has this been tested?

  1. Test with any theme running WordPress 5.9 RC3.
  2. Add a Social Icons block to a new page and add a few individual icons.
  3. In the sidebar, toggle on "Show labels", you should now see the service names.
  4. On one icon, add a custom label and see that it updates the label in the Editor.
  5. View on the frontend and confirm that the labels display correctly.

Screenshots

socail-icon-labels

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@ndiego ndiego added [Type] Enhancement A suggestion for improvement. [Block] Social Affects the Social Block - used to display Social Media accounts labels Jan 21, 2022
@ndiego ndiego self-assigned this Jan 21, 2022
@ndiego ndiego marked this pull request as ready for review January 22, 2022 16:19
@ndiego ndiego changed the title Social Links: Add the ability to show/hide labels Social Icons: Add the ability to show/hide labels Jan 22, 2022
@Mamaduka Mamaduka self-requested a review January 22, 2022 17:13
Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, Nick.

I just did a quick code review but will do more detailed testing on Monday.

ndiego and others added 2 commits January 22, 2022 13:10
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@ndiego
Copy link
Member Author

ndiego commented Jan 22, 2022

Thanks for the review @Mamaduka I have made those changes and added comments. 🙌

@carolinan
Copy link
Contributor

carolinan commented Jan 25, 2022

Oh I like this.
Question: What value does the title attribute bring?
There has been an effort to remove the title attributes from Core. https://core.trac.wordpress.org/ticket/24766

@jasmussen
Copy link
Contributor

Nice one!

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

It looks like we'll have to label for links when showLabels is enabled.

Instead of adding another conditional for displaying aria-label, maybe we could use a different method?

I think Technique #1 from this post should fit our requirements - https://www.sarasoueidan.com/blog/accessible-icon-buttons/.

What do you think?

@ndiego
Copy link
Member Author

ndiego commented Jan 25, 2022

Question: What value does the title attribute bring?

Good point, I will remove that.

I think Technique #1 from this post should fit our requirements - https://www.sarasoueidan.com/blog/accessible-icon-buttons/.

Hmm this is interesting. So we make the label always present, but hide it visually if showLabels is toggled off. We can then remove the aria-label from the link if I am understanding this correctly.

@Mamaduka
Copy link
Member

Correct, we can conditionally add class="screen-reader-text" based on show label value.

cc @aristath

@ndiego
Copy link
Member Author

ndiego commented Jan 26, 2022

I have made the requested changes.

I also ran across a possible other a11y improvement, but have not made it yet. I don't think that the svg icons should have role="img". I read that if an svg does, then it should also have an aria-label. Does anyone with better a11y knowledge than me have thoughts on this?

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks, @ndiego.

Do you mind creating a separate issue for role="img"?

@ndiego
Copy link
Member Author

ndiego commented Jan 27, 2022

Do you mind creating a separate issue for role="img"?

☝️ @Mamaduka will do. I will also see if there are any other instance of role="img" in other blocks that might need updating.

This PR good to merge?

@Mamaduka
Copy link
Member

My understanding is that we only need role="img" if SVGs aren't decorative and convey information. But I'm not 100% sure in the Social Icons case.

This PR good to merge?

Good to go! 🚢

@ndiego ndiego merged commit 7e23960 into WordPress:trunk Jan 27, 2022
@ndiego ndiego deleted the try/add-labels-to-social-icons branch January 27, 2022 12:43
@github-actions github-actions bot added this to the Gutenberg 12.6 milestone Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social Block: Add option to display text label
4 participants