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

Media Picker v3 #9461

Merged
merged 112 commits into from
Apr 22, 2021
Merged

Media Picker v3 #9461

merged 112 commits into from
Apr 22, 2021

Conversation

nielslyngsoe
Copy link
Member

@nielslyngsoe nielslyngsoe commented Nov 27, 2020

This new media picker supports three new main features, Controlling accepted Media Types, Local Image Crops, and Copy/Paste.

This Media Picker will at some point replace the v2 Media Picker we have today. Most likely v2 will not exist in Umbraco v.9. Tomorrow it will just become an additional choice.

The features of this Media Picker is made clear by looking at the DataType configuration:

image

  • Limit picking to certain Media Types.
  • Better control of amounts of picked items.
  • Set media start-node & ignore user start nodes(same feature as from v2)
  • Local Image Croppings

Test notes:

  • Set local focal point on images.
  • Use local focus points in templates.
  • Setup image croppings, change, delete, add new ones.
  • Pick media and set local crops.
  • Use image croppings in templates.
  • Limiting accepted media types.
  • Set validation limits, min & ma or just min and just max.
  • Have pick multiple items turned on but limit the editor to pick maximum one item. (in this way the template still gets an IEnumerable)
  • Put Media Item in trash bin and see it presented in the picker.
  • Have more items picked than the validation allows
  • Test copying media items from one property editor to another.
  • Test that you cannot paste none-allowed media types.
  • Copy media in media picker v2 to v2.
  • Copy media from media picker v2 to v3.
  • copy media from v3 to v2.
  • copy a collection of media items(done in the property-actions-menu).
  • test pasting a collection of media items that contain none-allowed types.(should not be possible)
  • Test remove all media, from property-actions
  • copy non-image media item from v3 to media picker v2 setup to only accept Image. (should not be possible)
  • insert or paste media items at a certain spot in a multiple media picker.
  • Read the documentation for ideas on more test cases

Documentation PR:
umbraco/UmbracoDocs#3106

Additional fixes:
#9948
#10111

Note - breaking change
Please note that a subtle breaking change slipped into this PR, for details see: #11858

Copy link
Member

@elit0451 elit0451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C# parts look good. Found some minor things that I will fix and lest some questions.

src/Umbraco.Core/Constants-Conventions.cs Outdated Show resolved Hide resolved
src/Umbraco.Core/Constants-Conventions.cs Outdated Show resolved Hide resolved
src/Umbraco.Core/Constants-DataTypes.cs Outdated Show resolved Hide resolved
src/Umbraco.Core/Constants-DataTypes.cs Outdated Show resolved Hide resolved
src/Umbraco.Core/Constants-DataTypes.cs Outdated Show resolved Hide resolved
src/Umbraco.Core/Constants-PropertyTypeGroups.cs Outdated Show resolved Hide resolved
src/Umbraco.Web/Editors/MediaController.cs Outdated Show resolved Hide resolved
@madsrasmussen madsrasmussen merged commit 1a5b885 into v8/dev Apr 22, 2021
@madsrasmussen madsrasmussen deleted the v8/feature/AB7941-MediaPicker3 branch April 22, 2021 08:28
@madsrasmussen
Copy link
Contributor

What a wonderful new feature. Let's get this one merged! 🎈 🥳 🎉

@bjarnef
Copy link
Contributor

bjarnef commented May 6, 2021

@madsrasmussen after looking at #9887 I noticed the Media Picker v3 was included in the branch.

I had a quick glance at the new feature and wonder how the remove the crops here:

image

I can refresh the page, but may loose some already defined crops and when clicking "Add", it isn't possible to save the datatype or submit that row.

image

I could of course fill in some dummy data and afterwards remove the additional rows/crops, but it may not be obvious.
I would expect a remove button always to be available at each row/crop.

It could also be useful if Media Picker v3 and Image Cropper could share the prevalue editor to define crops, but would probably cause issues with the existing saved data for Image Cropper prevalues, so maybe in a v2 of Image Cropper.
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/views/propertyeditors/imagecropper/imagecropper.prevalues.html

@nielslyngsoe
Copy link
Member Author

@bjarnef good points. I guess the Remove button could be present at all times, but in order to make the "create"-flow intuitive, it should be named "cancel" for first-time editing of a crop.

I also agree on using this for Image Cropper, but we will need to ensure that its not a breaking change :-)

Both two great proposals, if you are in for it feel free to make PRs :-)

@bjarnef
Copy link
Contributor

bjarnef commented May 10, 2021

@nielslyngsoe I could look at a PR for this, but will probably wait until it is part of v8/contrib branch.

angular
.module("umbraco")
.component("umbMediaPicker3PropertyEditor", {
templateUrl: "views/propertyeditors/MediaPicker3/umb-media-picker3-property-editor.html",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: ".../MediaPicker3/..." is the wrong casing compared to the folder on disk. This has been changed in v9, due to issues on Linux.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, would it be a help to change that in v8 as well? for future merging? Feel free to do so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there are no benefits of doing so..

@seanhakbb
Copy link

Would it be a good idea to store alt-texts in the future, locally, just as these crops? Right now its a bit annoying that this feature is only available in RTE and Grid. We usually have fields (multiple / language since variantions on media is not available) for this on the media, but there are cases where the image alt context changes depending on the page and a textfield below the media picker is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants