Multi URL Picker enhancements, add Content property and option to hide target checkbox

[ronaldbarendse]

Prerequisites

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

Description

This is a Umbraco 8 re-implementation of PR #5449.

1. Content property on the Link model (as commented on #2323 (comment))
Adding the Content property was quite simple, as the MultiUrlPickerPropertyConverter already gets the content or media as IPublishedContent. This allows easy access to any additional properties, e.g. an overview image that's rendered with the linked content or a thumbnail (with alt-text) of the linked media item.

Steps to test:

• Add links to content, media and external URL on a 'Umbraco.MultiUrlPicker' property
• Check whether the correct properties on the Link model that's returned from the PVC are set:
  • Content should only be null for external URLs
  • Content.Key should have the same GUID as in the Udi

2. Only save manually entered link name
The current link picker overlay sets the link name to the selected content or media item, but this has the following problems:

• Entering a (custom) link name and then selecting a node overwrites the entered text (as the name/title input is shown before the node pickers, this is a quite common/understandable order of doing things);
• If the linked node name is changed, the link name is not automatically updated, so editors might have to find all referencing links to update the link name;
• Because of the above, translating content (by duplicating and editing or using other tools) requires more text to be changed and when changing the links to point to the duplicated child nodes, require the custom text to be stored somewhere, so it's not overwritten (this obviously only applies to translations using multiple root nodes and not when using culture variants).

The interface/AngularJS part of this feature need to be changed, but the PVC already sets the content name when the link title/name is empty. When this is ready, I'll add testing steps and mark this PR as ready to review. Stay tuned!

3. Add option to hide target checkbox
Some links shouldn't open in new windows or tabs, so editors shouldn't get distracted by the possibility to set it, especially when the target isn't rendered in the link (on the front-end). This is similar to hiding the anchor/query input.

[nul800sebastiaan]

Thanks so far! Before you continue on this: both of these sound like breaking changes in behavior might cause problems in upgraded website.

I also quickly skimmed the old PR and saw a new setting was added which we generally try to avoid.

[ronaldbarendse]

Currently, there aren't any breaking changes: only an additional Content property on the Link model and better handling of unsupported UDIs in the PVC.

I've only added the additional overlay setting useNodeName in the V7 PR to keep backwards-compatibility in case the overlay is used in other places that don't expect a non-empty link title (e.g. the RJP Multi URL Picker). I'll rewrite this in a way that doesn't require this setting, e.g. by inferring this when the currentTarget object contains a title property (besides name)...

The additional 'Hide target' setting on the data type can't be avoided and seems like a very sensible feature, as you might not want to use this and/or want to make it easier/cleaner/friendlier for editors.

The PVC can also be slightly optimized to only process the configured maxNumber of (valid) links, instead of adding all processed links to a list and then returning only a subset. I understand it needs to switch from a list to single item (as the return type changes when maxNumber is set to 1), but I don't get why it returns a subset: lowering the maxChars on a textbox property also doesn't truncate the text, right?

[ronaldbarendse]

How can the content type ever be an element if it's retrieved from the snapshots content/media cache?

[ronaldbarendse]

We're only returning here (and not in the foreach loop above) to make sure it always returns the correct value type. If all links are unpublished content, the list will be empty and it will correctly return null.

[ronaldbarendse]

I've added the option to hide the target checkbox and fixed the link picker display when the anchor/query is hidden, so this PR is ready to get reviewed! (tagging @nul800sebastiaan and @poornimanayar as this isn't a new PR)

FYI the option to only save the manually entered link name requires a lot more work, so I've removed that from this PR...

[ronaldbarendse]

This ensures the anchor is not populated from the URL when the anchor/query string field is hidden. Otherwise it's possible to enter a URL with an anchor/query string, submit and then not be able to edit this anymore (as it's split from the URL and that field is hidden).

An existing anchor value is kept however (it's only hidden after all), so this might result in duplicate anchor/query strings (the one in the URL, because it's not split anymore and the previously saved value).

[ronaldbarendse]

@nul800sebastiaan I've fixed the conflict on en.xml, although it now shows additions that are already in v8/contrib, so I don't know how that will merge now (just beware 😉)...

[ronaldbarendse]

@nul800sebastiaan I've fixed the conflict on en.xml, although it now shows additions that are already in v8/contrib, so I don't know how that will merge now (just beware 😉)...

I've manually removed the additions that were not a part of this PR and force-pushed the existing merge commit, so everything is in order again and ready for review/merge 👍

[ronaldbarendse]

Looks like the MultiUrlPickerValueConverter is also missing the DefaultPropertyValueConverter, so that results in the same issue as for Nested Content (#2918).

[ronaldbarendse]

Looks like the MultiUrlPickerValueConverter is also missing the DefaultPropertyValueConverter, so that results in the same issue as for Nested Content (#2918).

This is now also fixed in the latest commit 👍

[SimonAntony]

Looks like a pretty solid implementation here @ronaldbarendse - thanks for sorting it, any idea when it maybe approved?

[umbrabot]

Hi there @ronaldbarendse!

Thanks so much for your contribution! We wanted to let you know that we love the content! Unfortunately, we have spent some time looking at the open PRs for the last minor release of Umbraco 8 and we have decided that while the work is great, we have decided not to merge this.

We hope to make 8.18 an umbazing release so we took the time to assess each contribution during our CMS workshop. If you would like to know a little more about that process, take a look at this article.

We would, however, be super happy to merge this into Umbraco 9. If you would be happy to take a look at how you could adapt this for Umbraco 9, please raise a PR and we'll take a look! For info on how to get started with Umbraco 9, check out these docs.

In the meantime, a big #H5YR goes out to you for your fine work here.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂