Make sure Social icons links aren't empty and improve UI clarity #60047
Conversation
afercia
added
[Type] Bug
| @@ -58,7 +58,9 @@ const SocialLinkURLPopover = ( { | |||
| onChange={ ( nextURL ) => | |||
| setAttributes( { url: nextURL } ) | |||
| } | |||
| placeholder={ __( 'Enter address' ) } | |||
| placeholder={ __( 'Enter social media link' ) } | |||
There was a problem hiding this comment.
I don't think "Enter social media link" is the right placeholder text. Although this is the Social Links block, not all of these are 100% social media links. Meetup, Skype, Mail, to name a few.
Perhaps "Enter social link" is clearer.
There was a problem hiding this comment.
OK then I guess we have to remove 'media' from other descriptions as well, e.g. in the block cards:
There was a problem hiding this comment.
That's a bit more descriptive with "or site", but sure, I'm fine with that. It's not a particularly great description as-is, if you have ideas on that front.
| @@ -128,19 +130,19 @@ const SocialLinkEdit = ( { | |||
| <PanelBody | |||
| title={ sprintf( | |||
| /* translators: %s: name of the social service. */ | |||
| __( '%s label' ), | |||
| __( '%s link text' ), | |||
There was a problem hiding this comment.
I actually think we don't need the social service name here, as it's clear that this is the %s block. Perhaps just the standard "Settings" label is more than enough.
There was a problem hiding this comment.
As an aside, we could probably make the PanelBody start open, like most of the other blocks. There's no reason to really hide it here.
There was a problem hiding this comment.
As an aside, we could probably make the PanelBody start open, like most of the other blocks. There's no reason to really hide it here.
Thanks @richtabor yes good point, will look into it.
For the labels, I'd lean towards 'link text' as that's how most users call a link text.
There was a problem hiding this comment.
I actually think we don't need the social service name here,
Nice, that would also simplify the label and avoid strings with variable placeholders like __( '%s label' ). Those should always be avoided because they're not fully translatable. They are problematic when translators need to swap the order or adjust to genre etc. Example from the current Italian translation:
There was a problem hiding this comment.
For the labels, I'd lean towards 'link text' as that's how most users call a link text.
I'm not sure I'd characterize this as terminology that most users use, but I'm ok with it.
There was a problem hiding this comment.
I would like to remind everyone 'Link Text' is what users are familiar with since quite a few years as that's what Classic editor always used:
| socialLinkName | ||
| ) } | ||
| initialOpen={ false } | ||
| > | ||
| <PanelRow> | ||
| <TextControl | ||
| __nextHasNoMarginBottom | ||
| label={ __( 'Link label' ) } | ||
| label={ __( 'Link text' ) } |
There was a problem hiding this comment.
We don't use "Link text" as a control label elsewhere. Perhaps we should just use "Text" and allow for the help prop to communicate what it is.
LinkControl uses "Text" and the navigation link block uses "Label" (which needs to be changed to "Text", to map to the LinkControl). I suspect we can do the same here for consistency.
There was a problem hiding this comment.
We don't use "Link text" as a control label elsewhere.
We should. See also #59993
There was a problem hiding this comment.
I disagree. In LinkControl, it's duplicative to have "Link text" and "Link URL" when you're opening/editing a link.
But what we should do here is the same controls as the Navigation Item blocks (like Custom Link), where you have the same "Text" and "Link" fields, in the same order.
| @@ -203,7 +203,7 @@ export function SocialLinksEdit( props ) { | |||
| /> | |||
| <ToggleControl | |||
| __nextHasNoMarginBottom | |||
| label={ __( 'Show labels' ) } | |||
| label={ __( 'Show links text' ) } | |||
There was a problem hiding this comment.
"Show links text" doesn't quiet land comprehension-wise. "Show labels" is much more understandable, even though it's not consistent verbiage. Perhaps "Show text" is sufficient.
| help={ __( | ||
| 'Briefly describe the link to help screen reader users.' | ||
| 'The link text is only visible when Show links text is enabled.' |
There was a problem hiding this comment.
Perhaps something a bit clearer, like "Visible when enabled from the parent Social Icons block."
| @@ -21,7 +21,7 @@ function render_block_core_social_link( $attributes, $content, $block ) { | |||
|
|
|||
| $service = ( isset( $attributes['service'] ) ) ? $attributes['service'] : 'Icon'; | |||
| $url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false; | |||
| $label = ( isset( $attributes['label'] ) ) ? $attributes['label'] : block_core_social_link_get_name( $service ); | |||
| $label = ( ! empty( $attributes['label'] ) ) ? $attributes['label'] : block_core_social_link_get_name( $service ); | |||
There was a problem hiding this comment.
Sorry! I had already created a PR for this, with an extra check in edit.js. It's been merged, but I attributed you in the commit.
There was a problem hiding this comment.
afercia
force-pushed
the
fix/social-icons-empty-links
branch
from
dff5402
to
f637158
Compare
|
Size Change: +956 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Looks like there is a problem with the 'Mail' icon and text color. Not sure why. Will double check all social icons as well. Update: this happens with the following social links:
They're the only ones that by default have dark text on light background. As such, they behave differently when setting a background color for all the social links, which makes it a bit useless as theny users are forced to set again also the text color. Will create a separate issue. Screenshot:
|
|
As a final improvement, I would like to prevent users from saving a link text made of only spaces and fallback to the social name. Will look into it tomorrow. |
|
In the latest commit I'm trying to make the editing UI reflect what happens on the front end, where an empty link text now always fallbacks to the social name. I'd appreciate some eyes on it and any better idea is welcome. Basically:
Screenshot of the input placeholder that shows the default to the social name:
|
afercia
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@WordPress/gutenberg-core I'd appreciate a review, when you have a chance 🙇🏽♂️ |
| } | ||
| placeholder={ socialLinkName } |
There was a problem hiding this comment.
Thank you, @afercia!
The changes test well and look good to me from the code perspective ✅
I see there's an ongoing discussion regarding new input labels. I'll defer to you and @richtabor on that one.
|
@Mamaduka 👋🏽 thanks for your review.
I think I already mentioned that's somehow an inaccurate feedback. WordPress always used the terminology 'Link text' since ages. That is what users have been used to see for years in the Classic Editor. I'm not sure looking only at Gutenberg is the best way to provide users with what they expect. Screenshot of the link editing dialog in Classic Editor:
|
|
@afercia, I rarely have strong opinions about the labels. I am happy to trust the a11y and the design teams :) @richtabor, can you have another look on proposed label changes? 🙇 |
|
Lack of or late feedback should not block this improvement. I'm going to merge it. Labels can always be discussed later and adjusted in the scope of a broader audit of the link editing labels. |
My feedback is accurate. The classic editor is not the same experience. We should not be making inconsistent one-off changes based on much older iterations. I am always on the side of consistency, with a holistic approach to revising. We don't use "Link text" anywhere in the current WordPress editor. |
On a general note, honestly I think all contributors should be more open to the fact that other contributors may have different ideas. Disagreement in an open source project is totally fine. Sharing different ideas may even lead to better results. What is not great in a collaborative open source project is to pretend to be always right. To me, changing long established conventions based on personal opinions isn't great for users. I'm not opposed to changes a priori. But, changes that are only based on personal opinions should at the very least be validated with some user testing. In WordPress, the text of a link is called 'Link text' and I don't see any good reason to change it. |
It's not about being right or wrong. Opinions aside, throughout the WordPress editor it's not called "Link text" — except for this one instance. That's objectively a departure from the rest of the editor experience, which I consider a regression. |
|
I agree the terminology should be consistent throughout the entire WordPress user interface. That's not only the Gutenberg editor. Looking only at the editor doesn't help providing users with a consistent experience. |
Fixes #60044
What?
Social links can be empty when users save the link text input field with an empty value or only spaces.
Why?
Empty links are far from ideal for semantics. SEO, and accessibility.
How?
SocialLinkURLPopoveroutside of the social linkButtonso that the tooltip of the button within the popover works.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast