-
Notifications
You must be signed in to change notification settings - Fork 32
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
PCH Smart Linking: Fix removal of existing Smart Links on subsequent runs #2430
Conversation
WalkthroughThe update in the code primarily enhances the smart linking feature within the editor sidebar. It improves how block content is accessed and smart links are applied, optimizing the overall functionality and user experience in managing links based on content and attributes. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This is related and closes #2422 |
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.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
src/content-helper/editor-sidebar/smart-linking/component.tsx (1)
5-5
: Consider removing theeslint-disable-next-line import/named
directive if it's no longer necessary.
Some questions/thoughts:
|
Thank you for your questions!
That's a good question. If we are going to push this to 3.14.3 base, it might have conflict with a few recent PRs, namely the #2401 (provider refactoring) and API retries (#2386). Although it should be relatively easy to fix manually.
Yes, that is correct! The user-defined links were never a problem, since they were included in the block's
Yeah, agreed. I think it should, since this function - |
Update: it works as expected in 6.3, validated with a wp-now environment 🙂 |
I think the way I'd approach this is to just branch from 3.14.3 and add the updated function/code there, with any needed adaptations. Attempting to merge the current work could bring a lot of code in, and more points of failure which we don't want in a patch release. I hope I'm making sense. What do you think? |
Sounds good to me, yes! |
Hello, I see you re-requested a review. I think it might make more sense to make create and merge the 3.14.4 PR first, just in case this PR becomes redundant. No issue approving though 🙂 |
Looking into this because I see it having conflicts but I wouldn't expect it to. |
@vaurdan, after merging |
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.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- src/content-helper/editor-sidebar/smart-linking/component.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- src/content-helper/editor-sidebar/smart-linking/component.tsx
Sounds good to me! Closing this one. |
Description
It was reported that when running the Smart Linking a second time in the same block would remove the existing Smart Links from that block and insert the new ones. This seems to be happening since at least WP 6.5.3.
This PR changes the how we were obtaining the block content - using
originalContent
- and now it uses the more appropriategetBlockContent
function to get the most up-to-date content for that given block.Motivation and context
Improve the reliability of the Parse.ly plugin and the Smart Linking feature in the latest WordPress release.
How has this been tested?
Tested locally by generating Smart Links on the same block multiple times, none of these tests resulted in the removal of any existing smart links.
Summary by CodeRabbit