LinkControl: refined the display of the link preview title and url when both are same #61819
Conversation
|
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. |
afercia
added
[Type] Enhancement
Thanks for your hard work @amitraj2203 🙌 Introducing new public API functions to another package could take more time as we want to make sure to design them well. In the meantime, I don't think that's a necessity to fix this issue. So I suggested above, feel free to fix this inline in the component (by introducing a local, unexported helper function), and we can consider abstracting this to public API in another PR. That will help fix the issue at hand and not delay it further. |
|
Thanks for having a look at it. I have made the changes and considered the regex which only removes the protocol and domain prefix |
There was a problem hiding this comment.
Thanks for your hard work on it @amitraj2203 🙌
I think there's some last-minute cleanup to do before moving forward with this one.
packages/block-editor/src/components/link-control/link-preview.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/link-control/link-preview.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think this is really close, and left a last round of feedback.
Let me know what you think @amitraj2203 and thanks again for working on this one!
packages/block-editor/src/components/link-control/link-preview.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/link-control/link-preview.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks great, thanks @amitraj2203 🙌
Just one last thing before we can ship this.
There was a problem hiding this comment.
LGTM 👍
Thanks again @amitraj2203 🙌
I'll let @afercia ship this after confirming #60900 is resolved as expected.
|
@amitraj2203 @tyxla I loved seeing your collaboration and perseverance on this PR ❤️ Thanks both. I can't speak for the code part and I'm totally confident in your expertise. Thanks for adding a few tests. |
…en both are same (WordPress#61819) * Add removeProtocol function to URL package * Fix redundant link preview display when link text includes protocol in LinkControl. * Changed regex for stripDomainPrefixes to handle all protocols * Add unit tests for stripDomainPrefixes function * Refactor URL filtering logic * Change variable name * Moved filterTitleForDisplay outside of component * Adds test cases for the updated regex * Fix typos and remove redundant tests for filterURLForDisplay * Fix isUrlRedundant check in LinkPreview component * Add test case for empty or falsy URL in filterURLForDisplay function Co-authored-by: amitraj2203 <amitraj2203@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
What?
Fixes: #60900
Why?
When the link text contains the protocol, the preview displays both lines (text and link) and they contain the same information, the only difference is the displayed protocol. So instead of displaying it twice its better to show it only once.
How?
This PR adds a check that weather the URL and Text is same after removing the protocol from the text.
Testing Instructions
https://test.org.https://test.organd add a link to ittest.orgwithout the protocol part.Screenshots or screencast
Screen.Recording.2024-05-21.at.5.53.40.PM.mov