Better crop mechanism for the grid image editor

[lars-erik]

Prerequisites

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

Description

Relies on #8014. Obsoletes #8013.

Replaces the old "focal point" crop mechanism in the grid with the same mechanism that is on the imagecropper datatype. Keeps code to support the focal point mechanism in views and front-end in order not to break existing solutions.
Also fixes black padding on images with config.size but no focalPoint selected. (Ex. default 0.5,0.5 shown is not respected)

Before - OOTB on v8/contrib with default starter kit:

https://youtu.be/wYqqGnKIVtI

After - OOTB on v8/contrib with default starter kit:

https://youtu.be/4orR6iI-Wao

For another time, I'll try to bring back aspect ratio, but would have to change from entityResource to mediaResource, which lost its search, so won't work without more changes. When we have aspect ratio, we can make the crop GUI center on the image instead of cropping from 0,0.

[emmaburstow]

Hey Lars!

Wonderful to see you here 😊

We'll take a look at the work and if we have questions, we'll ask. Speak soon,

Em

[lars-erik]

Cheers, @emmaburstow! :)

I've been wondering whether it should be introduced as a new editor type, compatible with the old one. Otherwise it'll be breaking since Media.cshtml have to be changed and often is not on upgrade.

L-E

[lars-erik]

It works again with #8048 merged.
Would still like to fiddle a bit more to see if it's possible to pass the existing value all the way in, but it would also be nice to know if this is at all considered merged, @nul800sebastiaan. ;)
Still also have concerns about changing behavior this much without config. Open for feedback on that one as well.

[lars-erik]

I've managed to have the picker open the details dialog when you already have a selected image with a crop. The RTE picker should not be affected (ie. work as before w/preview). A bug with both in the current Umbraco version is that they won't change folder to the image's folder. I might visit that before finishing up.

Still trying to have the cropper center initially, and likewise use the existing focal point when editing an "unset crop" in the media section. Seems like someone forgot about the existing focal point when making the cropping directive. Need to do some slightly complex math to get it done, but think I'll be able.

Also still unsure about modifying the existing media view, but also still open for suggestions.

Not to mention the fact that I'm starved for attention here.
I'd happily have it pulled as it is and continue the above in other PRs. The current version in this PR is miles better than the current version of Umbraco. (Except for the last point about the view)

[lars-erik]

There!

Image cropper property editor now initially centers crops on focal point.
Cropped grid media centers at .5, .5 when selected.

RTE and Grid media pickers change folder to the parent of items when you pop the dialog for existing selected/cropped media.

Here's a new demo showing the changes from the previous one:
https://youtu.be/RuKx-_lkOXI

Then we're left with what to do with the razor view for the grid media editor.
I had a thought that the grid editor could have a config setting to use the "new" version.
New Umbraco installs have the setting set to true, while old installs have to manually set it to true in order to use a new view. Or just swap the view to a new "media-v2.cshtml" or something. Same procedure in any case.

Thoughts?

PS. Couldn't help myself but to standardize some whitespace. Luckily GH now has "ignore WS changes". 😇

[nul800sebastiaan]

Sorry @lars-erik I've not read all of the comments yet, but I want to put your mind at ease about something:

Otherwise it'll be breaking since Media.cshtml have to be changed and often is not on upgrade.

People need to update this file during an upgrade, this is not something that would be considered a breaking change.

[lars-erik]

Cheers @nul800sebastiaan. Then I'll let it rest for now.
Fairly happy with the result now, and fairly confident it doesn't break anything else in the backoffice nor front-end markup or razor. I realize Bjarne's changes aren't merged either, so I guess I'm second in line. 😆

[nul800sebastiaan]

👍 There's a "few" more that need merging.. 🙈 Or .. do you mean any of Bjarne's changes are related to this?

[lars-erik]

I've moved my code to the files he PR'd in #8048
Otherwise up-to-date on yesterday's contrib-branch.

[emmaburstow]

What a wonderful improvement :) Thanks for the work Lars. You're a ⭐

[nul800sebastiaan]

I've moved my code to the files he PR'd in #8048

Sorry, I don't understand, does this require PR #8048? Do you conflict with that PR? Please help me with what you need here?

[lars-erik]

@nul800sebastiaan @bjarnef alerted me he'd moved the code I was changing in his #8048. So I merged that and now this PR requires #8048. I can move it back, but his changes seem legit to me.

[lars-erik]

Although I'm fairly certain merging this PR would automerge #8048 as well. 😊

[nul800sebastiaan]

I do apologize for the delay @lars-erik - it took me way longer than I wanted to to evaluate this one. This is super cool!!

Next time I would love for you to post some steps to test in text, this bit in grid.editors.config.js was quite crucial and difficult to copy/paste from your youtube video 😉

    {
        "name": "Image",
        "nameTemplate": "{{ value && value.udi ? (value.udi | ncNodeName) : '' }}",
        "alias": "media",
        "view": "media",
        "config": {
            "size": {
                "width": 500,
                "height": 500
            }
        },
        "icon": "icon-picture"
    },
    {
        "name": "Portrait Image",
        "nameTemplate": "{{ value && value.udi ? (value.udi | ncNodeName) : '' }}",
        "alias": "media.portrait",
        "view": "media",
        "config": {
            "size": {
                "width": 450,
                "height": 800
            }
        },
        "icon": "icon-picture"
    },
    {
        "name": "Landscape Image",
        "nameTemplate": "{{ value && value.udi ? (value.udi | ncNodeName) : '' }}",
        "alias": "media.landscape",
        "view": "media",
        "config": {
            "size": {
                "width": 800,
                "height": 450
            }
        },
        "icon": "icon-picture"
    },

Other than that code looks great, everything works and nothing blows up if people forget to update Media.cshtml so I'm very happy with this one! 👍 Thanks a million!!

[lars-erik]

Cheers for the merge! 🙌😁

Our ever vigilant super-hero friends @bjarnef / @kjac found a wee thing to check out. Will hopefully add another PR to fix said thing suddenly this week.

[arknu]

Just had a read-through of this and this seems like a downgrade to me. Focal-point cropping makes it possible to have different crops of an image based on for example screen size. I have a custom Media.cshtml that can do this, by rendering a different size on smaller phones. With the removal of the focal point crop, this is no longer possible. Having the option to do both is fine (like in the Image Cropper property editor), but removing focal point is not a good idea, in my opinion.

[lars-erik]

@arknu
@nul800sebastiaan reintroduced the focal point just because of this. It should still be there / possible to use?
See #8620 (comment)

My grief about this is that you get no feedback in the editor, and the preview was broken when you had set a crop.
I wonder if we could store a focal point even tho we now use percent as well.
That would mean you could get a "default crop" as this now provides and then you can do whatever with the focal point after.

Thoughts?

[nul800sebastiaan]

Just to make it clear: this ONLY gives you a cropper if you setup specific config for it manually. Otherwise you keep seeing the focal point (and as a bonus, editing the focal point now for the first time actually works).

If we broke something with the focal point, however then please let me know @arknu so we can have a look.

[arknu]

@nul800sebastiaan That sounds alright then :) I can just rename the config to something else. I still think that being able to specify a width and height and a focal point should be possible, but that easily be solved with a custom grid editor.