Skip to content
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

Add Multi Url Picker to core #2323

Merged
merged 6 commits into from Jan 20, 2019

Conversation

Projects
None yet
6 participants
@rasmusjp
Copy link
Member

rasmusjp commented Nov 13, 2017

PR for U4-8930


var udi = new GuidUdi(entityType, entity.Key);

string contentTypeAlias = (string) entity.AdditionalData["ContentTypeAlias"];;

This comment has been minimized.

Copy link
@bjarnef

bjarnef Nov 13, 2017

Contributor

There is a semicolon too much here :)


if (link.Trashed == false)
{
link.Url = umbHelper.TypedMedia(entity.Id).Url;

This comment has been minimized.

Copy link
@bjarnef

bjarnef Nov 13, 2017

Contributor

Should this line has a null check? E.g.

var media = umbHelper.TypedMedia(entity.Id);
if (media != null) 
{
     link.Url = media.Url;
}

This comment has been minimized.

Copy link
@rasmusjp

rasmusjp Nov 14, 2017

Author Member

Shouldn't be necessary because of the entity null check earlier, but I've added it anyways since the media index/cache could be corrupted or return null for some reason

@rasmusjp rasmusjp force-pushed the rasmusjp:feature/multi-url-picker branch from 8c0bb8b to 14cbece Oct 25, 2018

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

nul800sebastiaan commented Oct 25, 2018

Some notes from the quick test:

  • "Open" button should be renamed to "Edit" (or "Edit link"?)
  • Drag & drop drags all of the links
  • We could obsolete the original RJP version so it doesn't appear twice in the dropdown
@rasmusjp

This comment has been minimized.

Copy link
Member Author

rasmusjp commented Oct 25, 2018

  • "Open" is now "Edit"
  • Drag and drop is fixed (had an extra div in there)
  • RJP.MultiUrlPicker is now marked as obsolete

image

rasmusjp added some commits Oct 25, 2018

nul800sebastiaan added some commits Jan 17, 2019

@nul800sebastiaan nul800sebastiaan force-pushed the rasmusjp:feature/multi-url-picker branch from 36fd63b to 28a8d69 Jan 17, 2019

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

nul800sebastiaan commented Jan 17, 2019

Note: I've updated some code and a rebase on dev-v7, this was a force push, so if you have an existing clone and need to work on it, make sure to delete the local branch and fetch it again.

@nul800sebastiaan nul800sebastiaan merged commit e1c9b18 into umbraco:dev-v7 Jan 20, 2019

2 checks passed

Cms Continuous #201901170009 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nul800sebastiaan

This comment has been minimized.

Copy link
Member

nul800sebastiaan commented Jan 20, 2019

Thanks so much @rasmusjp ! It took a little while but it's finally here, goodbye old RelatedLinks picker!

@prjseal

This comment has been minimized.

Copy link
Contributor

prjseal commented Jan 21, 2019

This is great news. Thanks @rasmusjp

if (UmbracoContext.Current == null)
return source;

var umbHelper = new UmbracoHelper(UmbracoContext.Current);

This comment has been minimized.

Copy link
@ronaldbarendse

ronaldbarendse Jan 22, 2019

Contributor

UmbracoHelper should be lazy initialized: if there are no links (empty array) or all links are external, it's not needed (but initializing is quite a performance issue, especially because it's done for every property).

This comment has been minimized.

Copy link
@sniffdk

sniffdk Mar 29, 2019

Contributor

As far as I can tell from the code, newing up the UmbracoHelper is a very cheap operation. Where do the performance issue you talk about stem from?

This comment has been minimized.

Copy link
@ronaldbarendse

ronaldbarendse Mar 29, 2019

Contributor

Even so, it's not required and it's another object that has to allocate memory and than be garbage collected.

This comment has been minimized.

Copy link
@ronaldbarendse

ronaldbarendse Mar 29, 2019

Contributor

It's probably even better to just use the UmbracoContext.Current.ContentCache.GetById() and UmbracoContext.Current.MediaCache.GetById(), as UmbracoHelper just wraps to these calls!

This comment has been minimized.

Copy link
@sniffdk

sniffdk Mar 29, 2019

Contributor

Indeed, no real need to use the helper wrapper 👍

@ronaldbarendse
Copy link
Contributor

ronaldbarendse left a comment

I've added some comments to the existing code. Some additional functionality should probably be added as new PR's:

  • Multiple levels (like Cogworks.Meganav) - would require changes to the Link model!
  • Enable/disable types (e.g. only allow content/external links)
  • Define start nodes and allowed types (like MultiNodeTreePicker)
  • Removing items with umbracoNaviHide...

namespace Umbraco.Web.Models
{
public class Link

This comment has been minimized.

Copy link
@ronaldbarendse

ronaldbarendse Jan 22, 2019

Contributor

Should there be a IPublishedContent property that exposes the content/media (e.g. public IPublishedContent Content { get; set; })? The MultiUrlPickerPropertyConverter already does the lookup, so it could easily set this property.

And doesn't the Udi already contain what type of entity it is (umb://document/{guid})? That way, the both Udi and Content properties could be used to get the link type.

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor

ronaldbarendse commented Feb 19, 2019

@rasmusjp @nul800sebastiaan Any thoughts on my comments (especially the UmbracoHelper performance issue)?

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor

ronaldbarendse commented Mar 29, 2019

Also, for everyone already using RJP Multi URL Picker, upgraded to Umbraco 7.14.0 and now get a lot of 'Link' is an ambiguous reference between 'RJP.MultiUrlPicker.Models.Link' and 'Umbraco.Web.Models.Link' errors in your views: this can be easily fixed by adding a type alias to the <namespaces> element in your Views\Web.config:

<add namespace="Link=RJP.MultiUrlPicker.Models.Link" />

This might make the transition a little easier 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.