Preserve user populated link title when selecting nodes in link picker

[matthewcare]

Prerequisites

• I have added steps to test this contribution in the description below

Description

Due to the layout of the link picker, you often fill in the Link Title before selecting a page to link to.
It's often frustrating when your content is then removed and replaced with the node name.

This PR addresses the problem, and only updates the Link title field if it is not populated OR, in the case of switching nodes, the previously selected node is equal to the field.

This is also applied for selecting media - the field is only updated with the media name if it is currently blank.

Edit: When watching the demo, I noticed that selecting a media item when you have a node selected doesn't cause it to be deselected. So I've also fixed that.

Here is a demo of how that looks:

[github-actions]

Hi there @matthewcare, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[emmagarland]

Hi again @matthewcare!

Thanks for your PR to preserve the user-populated link title when selecting nodes in a link picker.

One of our Core Collaborators team will check this out as soon as possible.

Emma

[emmagarland]

Hi @matthewcare

Just an update, since it is a slight behavioural change in the back-office, I'm just checking with HQ that this outcome is what we'd like.

Hopefully, I'll be in touch soon!

Thanks again,

Emma

[emmagarland]

Hi @matthewcare

Just to let you know, HQ has confirmed this would be great to have in. It turns out that @ronaldbarendse actually created PR #7533 to do this in v8, but it never ended up being merged! 🙈

In his PR, if you didn't explicitly set the name, it would show the node name as a placeholder text in the picker and not store any value. When retrieving the picked Link value, the Property Value Converter would then use the current node name, ensuring you don't get the outdated name (from when it got picked, and also means storing less data).

Is it possible to update your implementation to behave like this? That would be awesome if so. Please let us know if you have any questions about this; the PR from Ronald hopefully gives an idea.

Thanks again,

Emma

[matthewcare]

Sure, I can take a look at re-implementing the functionality from Ronald's previous PR.
The only remark I have is that the functionality of using the current node name only seems to be aparent if the text field is left blank. I couldn't see anything that informs the user as such though.

Without checking out the V8 PR and looking, @ronaldbarendse could you tell me what the idea was in regard to UX? (Leave blank for node name)

In my opinion the link name field should be considered an override. i.e, if blank then it shows node name in the case of content/media, or the raw url in other cases. Filling in the name field will override these defaults. But then how is this best shown to the user? @emmagarland does HQ have any further guidance, or should I just get stuck in and wait for feedback once I update the PR?

[ronaldbarendse]

@ronaldbarendse could you tell me what the idea was in regard to UX?

The original v7 PR has a screen recording showing how the UX works with the placeholder text: #5449. Rendering the node name as the placeholder text should be clear enough that it won't get stored and will be used as default/fallback value, but I'm open to improvements that would make this more explicit (e.g. 'locking' the link name to sync with the picked node name, manually copy and/or clear the link name, etc.)...

[matthewcare]

Okay, @ronaldbarendse / @emmagarland, I've reimplemented the original PR's functionality.
Hopefully it holds up all these years later!
Let me know what you think

[emmagarland]

Hi @matthewcare,

Thanks for the update and the changes you have made 👍

It's been a little while, sorry about that, so there are now some conflicts!

Are you able to resolve the conflicts easily enough? I've had a look but I think it would be safer if it was from yourself, when you get time again.

After that, we'll review and hopefully be able to merge if all good!

Thanks,

Emma 🙂

[emmagarland]

Hi @matthewcare

A few weeks ago, there was an update in merged PR #15003 that might affect how many code changes are needed in this PR.

For instance, the addition of a new method (I've checked and this is where the conflicts are) oKToUpdateLinkTargetName.

If you get chance, would you be able to clarify if this has indeed resolved the issue you were working on? Is there still code in this PR that is needed for the editor experience?

Thanks again for your work on this,

Emma

cc @ronaldbarendse

[matthewcare]

Hi @emmagarland

The PR which was merged has almost the exact funcitonality as my original PR.
It's unfortunate that it was merged so quickly when my PR was more than a month older.

The issue which my original PR raised have been fixed by the now merged PR, however it does not contain the changes you requested from Ronald's much older PRs.

To clarify, the merged code prevents the user populated name from being replaced when you select a node. This is similar to my original PR.

The changes you requested mean that this PR contains in addition, always getting the actual node name, rather than what the node name was at the time of selecting.

For replication steps:

• Don't fill in a link title field, and select a node
• Render the link name in a view
• Change the node name of the selected node
• Observe that the link name automatically updates in the view

Although it was you that requested the changes, I think that they definitely improve the editor experience and should still be considered, even if the original scope of this PR is no longer valid.

If you agree that the additional improvements to the UX are sensible, I'll look at resolving the merge conflicts :)

Thanks,
Matthew

[emmagarland]

Hi @matthewcare

Firstly, sorry that your original functionality on the PR was superceded. This does happen now and then due to the nature of PRs, and perhaps because this one raised more conversations internally it led to the delay. I've raised it with the core collaborators team so we can try and avoid this happening again. I'll come back to you with any process updates; perhaps we need to always make sure an issue is referenced on any PR so that it isn't at as high a risk of being duplicated? I'll let you know what @nul800sebastiaan and the team come up with.

However, the good thing is that your PR has definitely still got some scope for the UI changes recommended by @ronaldbarendse! I'll just wait for him to give the go ahead, but I agree that those seem like great editor improvements.

All the best, and thanks again for your ongoing efforts 🙂

Emma

[matthewcare]

No worries, with as many contributors as there are it's bound to happen!

[emmagarland]

Hi @matthewcare

Apologies for the delay.

It seems there are now some new conflicts introduced that I am not able to auto-resolve (the linkpicker.controller.js one).

I'll just check with HQ that we want this to go through and then we can look into how to fix the conflicts.

Best regards

Emma

[ronaldbarendse]

Besides a small optimization, these changes look good to me 💪🏻

I originally also moved the link title input to the top to align with how it's displayed and entered (see #5449 (comment)) and added a setting to hide the target checkbox, but those changes might require more discussion (so I'd recommend doing them in a separate PR)...

[matthewcare]

Thanks for the feedback @ronaldbarendse
@emmagarland have you checked with HQ at all? I read that there was a feature freeze on the backoffice ahead of V14, so maybe this PR won't make it after all?

Thanks,
Matthew

[emmagarland]

Hi @matthewcare,

Yes, @ronaldbarendse is from HQ, I'm presuming this will be included therefore Ronald?

Thanks,
Emma

[matthewcare]

Hi again,

Now that there is some movement here I've fixed the conflicts, which made your suggestions a bit out of date @ronaldbarendse.
Hopefully everything still looks good.
I purposefully didn't add the "hide target" and moving the input as, like you say, this would cause more discussion, and I'm eager to get this PR merged. I can look at those other things in a new PR if you'd like me to though :)

Thanks,
Matthew

[ronaldbarendse]

Thanks again @matthewcare! I've quickly reviewed your changes and made some slight adjustments (mainly fixing the media selection), so this should now be ready to get merged 💪🏻

[emmagarland]

Hi @matthewcare, thanks so much for your patience with this PR to preserve the user populated link title! And thanks to @ronaldbarendse for your updates.

Works for me when running locally. It has now been successfully merged for inclusion into v13.3.0! 🙌

Thanks again -

Emma