umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

8.8RC: New grid cropper config doesn't work from package manifest #8926

Closed nul800sebastiaan closed 3 years ago

nul800sebastiaan commented 4 years ago

We have a super cool new feature for cropping in the context of a grid editor! See https://github.com/umbraco/Umbraco-CMS/pull/8023

While testing and fixing an issue related to that new feature (https://github.com/umbraco/Umbraco-CMS/issues/8545) we discovered one issue:

When adding the new config option to a package.manifest, we don't get a cropper to work with, seems like the config isn't passed through correctly. Here's my ~/App_Plugins/GridTest/package.manifest:

{
    "gridEditors": [
        {
            "name": "Updated Media with Config",
            "alias": "media.portrait2",
            "view": "media",
            "icon": "icon-picture",
            "config": {
                "width": 800,
                "height": 450
            }
        }
    ]
}

When picking something I don't get the cropper

nocropper

Question

Is the focal point obsolete or not? It still works on the cropper datatype, which is where we'll indeed still need it: image

@bjarnef suggested it might be needed to disable the focalpoint if coming from the RTE, I don't see it being needed from the RTE, but maybe I'm missing something: image


_This item has been added to our backlog AB#8383_

nul800sebastiaan commented 4 years ago

Additionally, @bjarnef noted a good optimization: https://github.com/umbraco/Umbraco-CMS/pull/8620#discussion_r490113905

bjarnef commented 4 years ago

It seems to be a similar issue with default grid editors in starter kit.

grid.editors.config.js

[
    {
        "name": "Rich text editor",
        "alias": "rte",
        "view": "rte",
        "icon": "icon-article"
    },
    {
        "name": "Image",
        "nameTemplate": "{{ value && value.udi ? (value.udi | ncNodeName) : '' }}",
        "alias": "media",
        "view": "media",
        "icon": "icon-picture"
    },
    {
        "name": "Macro",
        "nameTemplate": "{{ value && value.macroAlias ? value.macroAlias : '' }}",
        "alias": "macro",
        "view": "macro",
        "icon": "icon-settings-alt"
    },
    {
        "name": "Embed",
        "alias": "embed",
        "view": "embed",
        "icon": "icon-movie-alt"
    },
    {
        "name": "Headline",
        "nameTemplate": "{{ value }}",
        "alias": "headline",
        "view": "textstring",
        "icon": "icon-coin",
        "config": {
            "style": "font-size: 36px; line-height: 45px; font-weight: bold",
            "markup": "<h1>#value#</h1>"
        }
    },
    {
        "name": "Quote",
        "nameTemplate": "{{ value ? value.substring(0,32) + (value.length > 32 ? '...' : '') : '' }}",
        "alias": "quote",
        "view": "textstring",
        "icon": "icon-quote",
        "config": {
            "style": "border-left: 3px solid #ccc; padding: 10px; color: #ccc; font-family: serif; font-style: italic; font-size: 18px",
            "markup": "<blockquote>#value#</blockquote>"
        }
    }
]

image

nul800sebastiaan commented 4 years ago

@bjarnef That's expected, right? You can't crop those images so there's no need to show the cropper?

    {
        "name": "Image",
        "nameTemplate": "{{ value && value.udi ? (value.udi | ncNodeName) : '' }}",
        "alias": "media",
        "view": "media",
        "icon": "icon-picture"
    },

This has no config.

Am I missing something? 🙈

bjarnef commented 4 years ago

Just had a look at v8.6.3 where the same looked like this:

image

However in seemed when re-opening the image from grid the focal point was always in center.

But with recent changes it could maybe just show the thumbnail image here? Might be a help when editors fill the alternative text?

With <picture> element we might also want to have a configurable "Caption" input for <figcaption>, but something to look at later 😎

nul800sebastiaan commented 4 years ago

Aaah! I see! Good one! I wonder if the focal point was ever used for anything?

lars-erik commented 4 years ago

Aaah! I see! Good one! I wonder if the focal point was ever used for anything?

At the most the focal point has been a very confusing way to set the focal point of a crop that isn't visible when you set the point. As mentioned elsewhere, I'll be very surprised if someone's actually gone and implemented that in a media view.

I have been trying to show a preview of the image when there is no crop. (RTE+grid)
Seems that must've broken. I will hopefully get a chance to have a look next week.

nul800sebastiaan commented 3 years ago

Just to clarify, the remaining issue here is that when you have a package.manifest like so:

{
    "gridEditors": [
        {
            "name": "Updated Media with Config",
            "alias": "media.portrait2",
            "view": "media",
            "icon": "icon-picture",
            "config": {
                "width": 800,
                "height": 450
            }
        }
    ]
}

Then you will see the focal point when you pick the Updated Media with Config editor. This should be the full cropper, since you have a config with width and height. This is a feature that hasn't been implemented. This feature works if you add that config to the main grid.editors.config.js.

So the feature request here is: make the new cropper functionality (https://github.com/umbraco/Umbraco-CMS/pull/8023) also work for packages.

I'll put it in up for grabs for someone (hopefully @lars-erik) to pick up.

nul800sebastiaan commented 3 years ago

Well waddayaknow!! Thanks to @warrenbuckley I now see my error:

            "config": {
                "width": 800,
                "height": 450
            }

versus what it should have been:

        "config": {
            "size": {
                "width": 800,
                "height": 450
            }
        }

Was just missing the size.. this works perfectly for packages as well! 🎉

lars-erik commented 3 years ago

"Typos": A devs worst nemesis. 🤣 We always assume we, and what we review is syntactically correct.