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

Block Grid Editor #12826

Merged
merged 405 commits into from Oct 5, 2022
Merged

Block Grid Editor #12826

merged 405 commits into from Oct 5, 2022

Conversation

nielslyngsoe
Copy link
Member

@nielslyngsoe nielslyngsoe commented Aug 11, 2022

New Property Editor added to Backoffice.
This is based on the concept and feedback from #12578
This contains the Property Editor, Razor Rendering Method, A Layout Stylesheet, Example Blocks, and much more.

Please read the Documentation PR for more info:
umbraco/UmbracoDocs#4398

@nielslyngsoe nielslyngsoe marked this pull request as draft August 11, 2022 11:42
/// The settings.
/// </value>
[DataMember(Name = "settings")]
public IPublishedElement Settings { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

@nielslyngsoe should SettingsUdi and Settings be nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say yes, but lets see what @kjac says about that :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally yes - it would make a lot of sense, since the settings data might not be there. However, Settings are defined in the interface IBlockReference<TSettings> as not nullable, and that interface is shared between BlockGridItem and BlockListItem. In other words we'd introduce a breaking change for block list on BlockListItem, which is not entirely ideal at this point.


BlockGridLayoutAreaItem[]? areas = blockEditorData?.Layout?.ToObject<IEnumerable<BlockGridLayoutItem>>()?.SelectMany(ExtractLayoutAreaItems).ToArray();

if (areas?.Any() != true)
Copy link
Contributor

@bjarnef bjarnef Aug 15, 2022

Choose a reason for hiding this comment

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

It should be a bit faster to use Length here instead of any since it already knows the size of the array, while Any() iterate first element to check if there is any items (at least one) in the collection.

if (areas?.Length == 0)

Copy link
Member

Choose a reason for hiding this comment

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

It's not completely true.. Any() checks if the type implements ICollection and if so, it uses the known count.

There of cause is an overhead, but I'm not sure if it is worth if, compared to readability.

Below is the implementation of Any() from System.Linq

public static bool Any<TSource>(this IEnumerable<TSource> source)
{
    if (source == null)
    {
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source);
    }

    if (source is ICollection<TSource> collectionoft)
    {
        return collectionoft.Count != 0;
    }
    else if (source is IIListProvider<TSource> listProv)
    {
        // Note that this check differs from the corresponding check in
        // Count (whereas otherwise this method parallels it).  If the count
        // can't be retrieved cheaply, that likely means we'd need to iterate
        // through the entire sequence in order to get the count, and in that
        // case, we'll generally be better off falling through to the logic
        // below that only enumerates at most a single element.
        int count = listProv.GetCount(onlyIfCheap: true);
        if (count >= 0)
        {
            return count != 0;
        }
    }
    else if (source is ICollection collection)
    {
        return collection.Count != 0;
    }

    using (IEnumerator<TSource> e = source.GetEnumerator())
    {
        return e.MoveNext();
    }
}

Copy link
Contributor

@bjarnef bjarnef Aug 15, 2022

Choose a reason for hiding this comment

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

@bergmania okay, I think I have seende a similar explanation before regarding collections of type IList, ICollection etc.. but generally with enumerable collections it would perform a letter better.

Personally I prefer using Length or Count properties when I need to check whether there's no elements or at least one element.

It seems Array implements ICollection as well https://docs.microsoft.com/en-us/dotnet/api/system.array?redirectedfrom=MSDN&view=net-6.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Also remember that areas?.Length == 0 fails if areas is null, where areas?.Any() != true succeeds for areas being both null and empty. The equivalent using Length would be areas == null || areas.Length == 0, and I do belive that areas?.Any() != true is better for readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kjac yes, you're right. Mostly I use the ?.Count > 0 approach.

Some implements HasAny() or IsEmpty() extention method to handle both null and empty collections, but it may collide with extension methods in other libraries, so not sure if it makes things simpler in Umbraco core.

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.

Just a single comment this time around, everything else looks great 👍

src/Umbraco.Core/PropertyEditors/BlockGridConfiguration.cs Outdated Show resolved Hide resolved
@nielslyngsoe nielslyngsoe marked this pull request as ready for review October 5, 2022 07:40
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.

C# part LGTM 💪

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.

None yet

6 participants