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

Fix media tracking of items added via macro parameters in RTE and Grid #12139

Merged
merged 15 commits into from
Mar 21, 2022

Conversation

elit0451
Copy link
Contributor

Details

Fixes #8131 for Umbraco 9

After adding the item tracking improvements, there was still the issue that media items were not tracked when added via macro parameters in Rich Text Editor and Grid layout. @marcemarc had made a PR for Umbraco 8 resolving the issue (🎉 H5YR! 💪 ) and this is the fix for Umbraco 9.

Test

  • Have the SK installed
    • Create a Partial View Macro File called InsertFiles.cshtml ( ❗ Note: code below)

    • Allow the macro with the same name to be used in Rich Text Editor and the Grid

    • Add a macro parameter (ex: Files (files) : Umbraco.MultipleMediaPicker)

    • In a Rich Text area on a content node, insert the macro and pick a media item as the macro param

      • It should result in the following:
        image
    • Save and Publish

    • View the media item in the backoffice, and you should see that this is now referenced from this page

      • If you pick a folder from the Media section, all its contents will be listed in the macro, but the reference is made to the folder itself and therefore can be found on the folder's info app
    • You can do the same thing with the Grid

      • Either pick a Macro directly
      • Or add a RTE in a Grid and reference the macro from there

Contents of InsertFiles.cshtml:

@inherits Umbraco.Cms.Web.Common.Macros.PartialViewMacroPage
@using Umbraco.Cms.Core.Models.PublishedContent
@using Umbraco.Cms.Core
@using Umbraco.Cms.Core.Routing
@using Umbraco.Extensions

@inject IPublishedContentQuery PublishedContentQuery
@inject IVariationContextAccessor VariationContextAccessor
@inject IPublishedUrlProvider PublishedUrlProvider


@{ var mediaIds = Model?.MacroParameters["files"] as string; }

@if (mediaIds != null)
{
    <div class="row">
        @foreach (var mediaId in mediaIds.Split(','))
        {
            var media = PublishedContentQuery.Media(mediaId);

            @* a single image *@
            if (!media.IsDocumentType("Folder"))
            {
                <div class="col-xs-6 col-md-3">
                    <a href="@media.Url(PublishedUrlProvider)" class="thumbnail">@media.Name</a> [@System.IO.Path.GetExtension(@media.Url(PublishedUrlProvider).ToString()).Substring(1)]
                </div>
            }

            @* a folder with images under it *@
            foreach (var image in media.Children(VariationContextAccessor))
            {
                <div class="col-xs-6 col-md-3">
                    <a href="@image.Url(PublishedUrlProvider)" class="thumbnail">@image.Name</a> [@System.IO.Path.GetExtension(@image.Url(PublishedUrlProvider).ToString()).Substring(1)]
                </div>
            }
        }
    </div>
} 

Copy link
Member

@Zeegaan Zeegaan left a comment

Choose a reason for hiding this comment

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

Pattern matching is the new hotness 🔥

@Zeegaan
Copy link
Member

Zeegaan commented Mar 18, 2022

Great job! I've fixed up some of the small edge-cases but the rest LGTM! 👓

@@ -77,7 +77,7 @@ public IMacro GetByAlias(string alias)

public IEnumerable<IMacro> GetAllByAlias(string[] aliases)
{
if (aliases.Any() == false)
if (aliases.Any() is false)
{
return base.GetMany();
}
Copy link
Contributor

@bjarnef bjarnef Mar 18, 2022

Choose a reason for hiding this comment

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

@Zeegaan shouldn't it just use Length here since the aliases is string[].

for example:

if (aliases?.Length == 0)
{
    return base.GetMany();
}

As far I know Any() check first item if collection, but with Array, List, ICollection etc, Length or Count property already know the size of the array/collection.

Copy link
Member

@Zeegaan Zeegaan Mar 21, 2022

Choose a reason for hiding this comment

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

Hey @bjarnef, thanks for your comment, i had to check how this works 👀 When it is an array, Any() will also use the Count property like this:
image
And because of that, i think its more readable if we use Any() instead of Length

@Zeegaan Zeegaan merged commit 6f7a9cb into v9/dev Mar 21, 2022
@Zeegaan Zeegaan deleted the v9/bugfix/track-media-items-picked-as-macro-params branch March 21, 2022 07:24
Zeegaan added a commit that referenced this pull request Mar 21, 2022
…cked-as-macro-params

Fix media tracking of items added via macro parameters in RTE and Grid
@Zeegaan
Copy link
Member

Zeegaan commented Mar 21, 2022

Cherry picked for 9.4

@marcemarc
Copy link
Contributor

Thanks @elit0451 @Zeegaan !!!

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.

5 participants