skartknet / Iconic

The ultimate icon picker for Umbraco backoffice
MIT License
7 stars 8 forks source link

Support Delivery API in v12+ #44

Closed crjc closed 9 months ago

crjc commented 1 year ago

The IconicValueConverter needs to implement IDeliveryApiPropertyValueConverter for Umbraco 12 onwards. Without implementing this, the API currently returns null for all iconic values..

I implemented this interface and got it working fine. I'd create a PR however I'm not sure on the best way to not break support for older umbraco installations.

The implementation that worked for me was the following:

        public PropertyCacheLevel GetDeliveryApiPropertyCacheLevel(IPublishedPropertyType propertyType) => GetPropertyCacheLevel(propertyType);

        public Type GetDeliveryApiPropertyValueType(IPublishedPropertyType propertyType) => typeof(string);

        public object? ConvertIntermediateToDeliveryApiObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel referenceCacheLevel, object? inter, bool preview, bool expanding) =>
            ConvertIntermediateToObject(owner, propertyType, referenceCacheLevel, inter, preview)?.ToString();

(Use a string instead of HtmlString for the delivery API)

crjc commented 1 year ago

On second thought, it would probably be more appropriate to return something like this for the delivery API:

    public class IconicApiResponse
    {
        public string Icon { get; set; }
        public Guid? PackageId { get; set; }
        public string? PackageName { get; set; }
        public string? FrontendTemplate { get; set; }
    }
        public PropertyCacheLevel GetDeliveryApiPropertyCacheLevel(IPublishedPropertyType propertyType) => GetPropertyCacheLevel(propertyType);

        public Type GetDeliveryApiPropertyValueType(IPublishedPropertyType propertyType) => typeof(IconicApiResponse);

        public object? ConvertIntermediateToDeliveryApiObject(IPublishedElement owner,
            IPublishedPropertyType propertyType, PropertyCacheLevel referenceCacheLevel, object? inter, bool preview,
            bool expanding)
        {
            if (inter == null) return null;
            var result = new IconicApiResponse();

            var icon = (SelectedIcon)inter;
            result.Icon = icon.Icon;

            var packages = _configuredPackages.GetConfiguratedPackages(propertyType);

            if (icon != null && packages.ContainsKey(icon.PackageId))
            {
                var pckg = packages[icon.PackageId];
                result.PackageId = pckg?.Id;
                result.FrontendTemplate = pckg?.FrontendTemplate;
                result.PackageName = pckg?.Name;
            }

            return result;
        }
skartknet commented 1 year ago

Hi @crjc , thanks so much for this! I'll use your code and give it a try.

skartknet commented 1 year ago

I managed to return some values:

"properties": {
        "icon": {
            "icon": "agriculture",
            "packageId": "7531b17e-1741-4c17-a4da-2ae39e7aa53c",
            "packageName": "Material Icons",
            "frontendTemplate": "<i class=\"material-icons\">{icon}</i>"
        }
 },

But I'm wondering if it doesn't make more sense to return the value ready to use like

"properties": {
        "icon":  "<i class=\"material-icons\">agriculture</i>"
        }
},

I don't see the benefit of getting all the internal details about the package.

In regards to supporting previous versions, I think that;s not possible, and in fact, it would be good to clean up some other legacy code, so I think I will go with just releasing a newer 12.0.0 version. This would also follow the versioning strategy of other packages.

crjc commented 1 year ago

With headless websites, I imagine it’s more useful to be able to retrieve the icon class/name, not the raw html. My original post returned that ready to use value, but this made it more awkward for me to consume iconic on a react site.

I’m not using the package id/name, but I included them just in case anyone would have a use for them. So maybe the properties only needs to be something like:

"properties": {
        "icon": {
            "name": "agriculture",
            "html": "<i class=\"material-icons\">agriculture</i>"
        }
 },
skartknet commented 1 year ago

I was looking into that from the wrong point of view. Of course, a headless cms doesn't know the platform your using the data on. So having a frontend template should actually be optional.

This will change bit the configuration of the property, as having a frontend template is currently mandatory.

Going back to the API, I think the values returned should be the icon name, package name, and package id, and not a template as this data can potentially be consumed in many different platforms.