Skip to content

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

Merged
ndiego merged 6 commits into
WordPress:trunkfrom
ndiego:try/add-labels-to-social-icons
Jan 27, 2022
Merged

Social Icons: Add the ability to show/hide labels#38152
ndiego merged 6 commits into
WordPress:trunkfrom
ndiego:try/add-labels-to-social-icons

Conversation

@ndiego

@ndiego ndiego commented Jan 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 requested review from ajitbohra and mkaz as code owners January 22, 2022 16:19
@ndiego ndiego requested review from aristath and jasmussen January 22, 2022 16:20
@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
Mamaduka
Mamaduka reviewed Jan 22, 2022
View reviewed changes
Comment thread packages/block-library/src/social-link/edit.js Outdated
Mamaduka
Mamaduka reviewed Jan 22, 2022
View reviewed changes
Comment thread packages/block-library/src/social-links/edit.js
Mamaduka
Mamaduka reviewed Jan 22, 2022
View reviewed changes
Comment thread packages/block-library/src/social-link/index.php Outdated
Mamaduka
Mamaduka reviewed Jan 22, 2022
View reviewed changes

@Mamaduka Mamaduka left a comment

Copy link
Copy Markdown
Member

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
@ndiego @Mamaduka
Use nullish coalescing operator.
42b4555 
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@ndiego @Mamaduka
Use esc_html over esc_attr.
255e113 
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@ndiego

ndiego commented Jan 22, 2022

Copy link
Copy Markdown
Member Author

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

@carolinan

carolinan commented Jan 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

jasmussen commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

Nice one!

Mamaduka
Mamaduka reviewed Jan 25, 2022
View reviewed changes

@Mamaduka Mamaduka left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

ndiego commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

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

Mamaduka commented Jan 25, 2022

Copy link
Copy Markdown
Member

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

cc @aristath

@ndiego

ndiego commented Jan 26, 2022

Copy link
Copy Markdown
Member Author

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?

Mamaduka
Mamaduka approved these changes Jan 27, 2022
View reviewed changes

@Mamaduka Mamaduka left a comment

Copy link
Copy Markdown
Member

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

ndiego commented Jan 27, 2022

Copy link
Copy Markdown
Member Author

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

Mamaduka commented Jan 27, 2022

Copy link
Copy Markdown
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
@ndiego ndiego mentioned this pull request Jan 27, 2022
Closed
@femkreations femkreations mentioned this pull request Apr 14, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mamaduka Mamaduka Mamaduka approved these changes
@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner
@mkaz mkaz Awaiting requested review from mkaz
@jasmussen jasmussen Awaiting requested review from jasmussen
@aristath aristath Awaiting requested review from aristath

Assignees

@ndiego ndiego

Labels

[Block] Social Affects the Social Block - used to display Social Media accounts [Type] Enhancement A suggestion for improvement.

Projects

None yet

Milestone

Gutenberg 12.6

Development

Successfully merging this pull request may close these issues.

Social Block: Add option to display text label

4 participants

@ndiego @carolinan @jasmussen @Mamaduka