| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat: clarify label & add help text with link #70451
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
Conversation
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @coderGtm! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
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. |
t-hamano
added
[Type] Enhancement
|
Hi @ellatrix Did you get a chance to look into this PR? I have clarified the purpose and the text as discussed and agreed upon by @Soean, @karmatosed and @skorasaurus in #20208 Is there anything else that I need to do? I am fairly new to contributions here so not sure. Thanks |
skorasaurus
added
[Focus] Accessibility (a11y)
|
Hi @coderGtm, I’ve tested this PR on on block editor and have put my testing report below. Machine: macOS Air (M3) SummaryThis report confirms that the proposed updates in PR work as intended. The changes enhance the clarity of the "Link Rel" attribute in the image block by updating the label and providing contextual help text with a link to relevant documentation. Scope of Change
Steps to Reproduce
Expected Results
Actual Results
Before and After Summary
|
|
@Mamaduka looping you in for a review here, to me this is ready to go so taking off the design review. I also approve first the code but I would like a second opinion just to be sure. |
|
I think this falls under the "Need Decision" label. I don't have a strong opinion; both labels work for me. Considering it's an advanced setting, users shouldn't just fill it out. Additionally, the terminology needs to be consistent across the project. The image link control isn't the only place with this field. Examples:
I would hold off making further changes until there's a final decision. |
packages/block-editor/src/components/url-popover/image-url-input-ui.js
Outdated
Show resolved
Hide resolved
|
I think this is a sensible change. The label is pretty unclear as it is, and improving the clarity of that field is valuable. It is true that relatively few people are likely to use it, but we might as well make it easier for those who do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderGtm, thanks for refreshing the branch. It appears that it now contains some unrelated changes.
Additionally, remaining feedback needs to be addressed, and this field needs to be made consistent across the editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to this file are unrelated to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
|
@Mamaduka I mistakenly merged another branch into this causing other PR's changes to be added here. I will remove them. Additionally, by remaining feedback do you mean applying this Link Rel changes everywhere in Gutenberg? |
That's right. The similar "Link Rel" fields should be consistent across the app. |
|
@Mamaduka I have made the changes across Gutenberg. Please take a look and let me know if anything else needs to be done. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @coderGtm!
The code changes look good and test well. I've left some notes regarding text copy, based on project guidelines. But I would defer to @karmatosed and @joedolson regarding it.
| __next40pxDefaultSize | ||
| __nextHasNoMarginBottom | ||
| label={ __( 'Link rel' ) } | ||
| label={ __( 'Link Relation' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| label={ __( 'Link Relation' ) } | |
| label={ __( 'Link relation' ) } |
I think similar labels use sentence case. See: https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/documentation/copy-guide.md#five-pay-attention-to-capitalization
| help={ | ||
| <> | ||
| { __( | ||
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' | |
| 'The link relation attribute defines the relationship between a linked resource and the current document.' |
I think we should use standard casing here. This matches casing in the MDN article.
Note: Label and text changes should be the same everywhere.
|
Thanks for pointing it out @Mamaduka and for taking the time to review and approve the PR. Reading the guidelines you linked, it seems the changes you suggested should be the way to go. I can still wait for @karmatosed and @joedolson for their nod so we are on the same page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one change I'd like to see: to improve the link text for more information.
| help={ | ||
| <> | ||
| { __( | ||
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid using meaningless link text like "Learn more". Can we instead place the link on the word "link relation attribute"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joedolson. I'd incorporate those changes. Btw, what do you think about the sentence case thing suggested by @Mamaduka earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka @joedolson I have got the link inline with element interpolation. From what I think, now it makes sense to keep the casings as it is since now "Link Relation" is a term that we refer externally directly by clicking on it. I am not sure though and not aware if any guidelines are there for such a case. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have a strong opinion here; I usually follow the text copy guidelines. We could keep casing in the help text, but let's fix it for the label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the label above the input box? That is capitalized automatically. All labels are like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #70451 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Done.
| help={ | ||
| <> | ||
| { __( | ||
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous request.
| help={ | ||
| <> | ||
| { __( | ||
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous request.
| help={ | ||
| <> | ||
| { __( | ||
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous request.
| help={ | ||
| <> | ||
| { __( | ||
| 'The Link Relation attribute defines the relationship between a linked resource and the current document.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous request.
| 'The <linkRel>Link Relation</linkRel> attribute defines the relationship between a linked resource and the current document.' | ||
| ), | ||
| { | ||
| linkRel: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'The <linkRel>Link Relation</linkRel> attribute defines the relationship between a linked resource and the current document.' | |
| ), | |
| { | |
| linkRel: ( | |
| 'The <a>Link Relation</a> attribute defines the relationship between a linked resource and the current document.' | |
| ), | |
| { | |
| a: ( |
Suggestion: We can just use `a as a link placeholder, it's a typical pattern in core, and translators might be more aware of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made these changes @Mamaduka
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow-ups, @coderGtm!
The changes look good to me. @joedolson, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little on the fence about the initial capitalization, but I don't think it's important enough to hold this up.
|
Nit: Personally, I'd be happy if the links were translatable. MDN has many pages localized into several languages. Site users may not be comfortable with English and would prefer to view resources in their own language: 
Example: gutenberg/packages/block-library/src/cover/edit/inspector-controls.js Lines 308 to 319 in 228c3b5
|
Labels
7 participants
What?
Closes #20208
This PR aims to add more clarity the Link Rel attribute in image URL input by:
Why?
It was discussed in #20208 to clarify the label and add helper text to clarify what the "link rel" is actually for. It was also proposed to add a link to https://developer.mozilla.org/docs/Web/HTML/Attributes/rel so users can get more information easily.
How?
ExternalLinkto link external documentation so users can get more info.Testing Instructions
Screen.Recording.2025-06-17.at.1.26.05.PM.mov
Screenshots