AaronSadlerUK / Our.Umbraco.UmbNav

Drag and drop menu editor for Umbraco V8, V9 and V10+
MIT License
10 stars 15 forks source link

Programmtically injecting umbNavItem #43

Closed mistyn8 closed 2 years ago

mistyn8 commented 2 years ago

Which exact Umbraco version are you using? For example: 8.7.0 - don't just write v8 or v9

9.0.1

Bug summary

I've got as far as getting an item injected, but can't see how to add a URL for a link

if (item.Title == "Sales")
                    {

                        var salepage = new UmbNavItem() {

                            Id = 0,
                            Udi = null,
                            Key = new System.Guid(),
                            Title = "new item",
                            Target = null,
                            Noopener = null,
                            Noreferrer = null,
                            Anchor = null,
                            Children = Enumerable.Empty<UmbNavItem>(),
                            Culture = null,                            
                            ItemType = UmbNav.Core.Enums.UmbNavItemType.Link,
                            DisplayAsLabel = false,
                            Image = null,
                            CustomClasses = null,
                            Level = item.Level,
                            IsActive = false,
                            Content = null

                        };
                        cchildren.Insert(i+1, salepage);
                        //cchildren.Remove(item);
                    }

I can see this is serialised link vs content

{
            "description": "/department-forms/classic-car-valuation",
            "title": "testing",
            "url": "/department-forms/classic-car-valuation",
            "children": [],
            "icon": "icon-link",
            "itemType": "link"
          },
          {
            "id": 2430,
            "udi": "umb://document/fdce71ab98064b028aa59823620e0ae9",
            "key": "fdce71ab-9806-4b02-8aa5-9823620e0ae9",
            "name": "Sales",
            "description": "/4x4s-cars-vans/sales/",
            "url": "/4x4s-cars-vans/sales/",
            "children": [],
            "icon": "icon-auction-hammer color-red",
            "published": true,
            "naviHide": false,
            "culture": "undefined",
            "itemType": "link",
            "collapsed": true
          },

Any ideas?

Specifics

No response

Steps to reproduce

as above

Expected result / actual result

No response

AaronSadlerUK commented 2 years ago

It uses the Url property to decide on what is output, so if the item is of type Media or Content then it will get the Url from the Content property, if it is any other type it returns the Url property:

https://github.com/AaronSadlerUK/Our.Umbraco.UmbNav/blob/develop/src/UmbNav.Core/Extensions/UmbNavItemExtensions.cs#L123

https://github.com/AaronSadlerUK/Our.Umbraco.UmbNav/blob/develop/src/UmbNav.Core/Models/UmbNavItem.cs#L67

Hopefully the two above links are helpful

mistyn8 commented 2 years ago

Is it because the Url property is internal.. so I can't access it to set it on umbNavItem? for a link??

AaronSadlerUK commented 2 years ago

Ah yeah probably, I did that to force the use of the Extension when rendering, I don't really want to make that public again, as I can forsee what will happen, especially on multi-lingual sites.

Open to other ideas though

mistyn8 commented 2 years ago

could we use a deserialiser... I presume there already is one to take

{
            "description": "/department-forms/classic-car-valuation",
            "title": "testing",
            "url": "/department-forms/classic-car-valuation",
            "children": [],
            "icon": "icon-link",
            "itemType": "link"
          }

this back to a umbNavItem?

AaronSadlerUK commented 2 years ago

That is done here: https://github.com/AaronSadlerUK/Our.Umbraco.UmbNav/blob/develop/src/UmbNav.Core/ValueConverters/UmbNavV8ValueConverter.cs#L62

mistyn8 commented 2 years ago

So this does work.. but feels dirty... Though it is interesting that internal properties can be deserialised to.

var json = "{\"description\": \"/department-forms/classic-car-valuation\", \"title\": \"123testing\", \"url\": \"/department-forms/classic-car-valuation\", \"children\": [], \"icon\": \"icon-link\", \"itemType\": \"link\"}";

var navitem = Newtonsoft.Json.JsonConvert.DeserializeObject<UmbNavItem>(json);
navitem.Level = item.Level;

cchildren.Insert(i + 1, navitem);

Had to manually add the level after deserialisation as that is set to be [ignored] from the json (as you will rightly be calculating it from the nesting).

cibis-adrian commented 2 years ago

We also have the same issue. It would be nice to have access to the full functionality of the class.

Because there is an extension to get the Url, perhaps the setter could at least be opened up?

[JsonProperty("url")] public string Url { internal get; set; }

There are many more properties that are internal which don't have a getter/extension method. Perhaps these could just be opened up?

AaronSadlerUK commented 2 years ago

For the Url property i do not want to allow it to be used for the rendering.

The .Url() extension is what's to be used to get the link due to the additional parts.

I can make this an internal set if it will solve the problem?

As for the other properties what ones would you need access to?

And can you provide some examples so I can test the change doesn't break other parts of the system.

Thanks!

As always PRs are welcome

cibis-adrian commented 2 years ago

Url is the only one we need access to (not sure on what other users may need or how they have things set up).

An internal getter with a public setter should do the trick (you mentioned internal set, it should be public set). That will keep access to it going through the extension method as you intend, but still allow these injected cases to work without resorting to deserialization or reflection.

AaronSadlerUK commented 2 years ago

I will have a look at this today

AaronSadlerUK commented 2 years ago

@mistyn8 / @cibis-adrian So what I have done in PR #49 is moved all the internal classes into a new class called UmbNavInternalItem this has a property called Url which has an internal getter.

The UmbNavItem still has an internal Url property which is used by the Extension, but cannot be rendered out.

If you think this solves your issue I am happy to merge, however it will be a breaking change for your implementations.

cibis-adrian commented 2 years ago

@AaronSadlerUK If the line of code I mentioned was applied directly on the UmbNavItem instead of splitting it into a new class, I believe everything would have just worked without it becoming a breaking change.

AaronSadlerUK commented 2 years ago

Yes, but i do not want the Url property available on the rendering class, which it was with that line of code.

This way the extension is still the only Url, and injection can happen

cibis-adrian commented 2 years ago

The Url properly won't be available for rendering, it would still be internal exactly like it was before. Try it out and see. I have a local build where I've made the change I mentioned and .Url is not available as a getter so it can't be used when rendering, you have to use the .Url() extension method. But you can still do .Url = "url" for injection cases because the setter is public.

Up to you, if you want it the way you've done and as a breaking change I guess that's fine as long as it works (not sure when I'll have the time to be able to test it).

AaronSadlerUK commented 2 years ago

I had a try the way you suggested, and the Url property was visible on the class, and I can forsee issues being raised that it doesn't work when people try to use it for rendering.

So I will go with my PR in the next version, the only change required in your code should be the class name from UmbNavItem to UmbNavInternalItem