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

Add ContainedStereotypes BagPartSettings to allow a user to include content-types by steryotype #11977

Closed
MikeAlhayek opened this issue Jul 8, 2022 · 16 comments · Fixed by #11978
Milestone

Comments

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Jul 8, 2022

Is your feature request related to a problem? Please describe.

When attaching BagPart to a content type, the user must explicitly set ContainedContentTypes with an array of content-types to be include in the BagPart. This is good for most case, however, it would be great to allow for setting the contained content-types using Stereotype. Stereotype would be in addition to not in place of ContainedContentTypes.

For example, I want to group all Contact Methods (phone number, address, email address... etc) by a stereotype type called ContactMethod. All of these content types share similar functionally which is a way to contact a person. Now I created a Person content-type and attached BagPart to it. In this case, I had to explicitly specific each content types that in the ContainedContentTypes. But if a new content-type was added later "from other feature/module", the user would have manually to edit the BagPart settings every time/every where ContactMethods are using to add the new content-type which it isn't efficient.

Additional benefit here if any module want to create a new content type that adhere to my Stereotype would be welcome to do so without having the need to modify the BagPart Settings. So now my BagPart is more flexible.

Describe the solution you'd like

Add a way to specific a stereotypes in the BagPartSettings in addition to ContainedContentTypes so the user is able to explicitly able to set contained-content-types or explicitly set the stereotype.

We can achieve this by changing the BagPartSettings to something like this

    public class BagPartSettings
    {
        public string[] ContainedContentTypes { get; set; } = Array.Empty<string>();
        public string[] ContainedStereotypes { get; set; } = Array.Empty<string>();
        public string DisplayType { get; set; }
    }

Then the line BagPartDisplayDriver will be changed to something like this

if(settings.ContainedStereotypes != null && settings.ContainedStereotypes.Length > 0)
{
    return _contentDefinitionManager.ListTypeDefinitions()
                   .Where(contentType => contentType.HasStereotype() && settings.ContainedStereotypes.Contains(contentType.GetStereotype(), StringComparer.OrdinalIgnoreCase));
}

 return settings.ContainedContentTypes
                .Select(contentType => _contentDefinitionManager.GetTypeDefinition(contentType))
                .Where(contentType => contentType != null);

Then these the first 2 extensions would be added to clean up the code whereas the rest of the helpers will be nice add to help with other usage


public static class ContentTypeDefinition
{
    public static bool HasStereotype(this ContentTypeDefinition contentTypeDefinition)
    {
        return !String.IsNullOrEmpty(contentTypeDefinition.GetSettings().Stereotype);
    }

    public static string GetStereotype(this ContentTypeDefinition contentTypeDefinition)
    {
        return contentTypeDefinition.GetSettings().Stereotype;
    }
}
@MikeAlhayek MikeAlhayek changed the title Add StereoTypes BagPatSettings to allow a user to include content-types by steryotype Add ContainedStereotypes BagPartSettings to allow a user to include content-types by steryotype Jul 8, 2022
@sebastienros
Copy link
Member

Stepping into unknown territory. Maybe someone will want all the content types of a stereotype excluding some specific ones ... if might get tricky.

But I am not disagreeing with the proposal, it's the simplest next step. Would the two properties work like a union between stereotypes and types? OR if not should we use a single property that would contain either a stereotype name or type?

@sebastienros sebastienros added this to the 1.x milestone Jul 14, 2022
@MikeAlhayek
Copy link
Member Author

@sebastienros i would think it’s one or the other… at least this is how the PR will do. If someone wants to cherry pick, they just don’t use the Stereotype.

@ns8482e
Copy link
Contributor

ns8482e commented Jul 14, 2022

BagPart is for Content stereotype only. Not sure what would be consequence of allowing multiple content types from multiple stereo types under same BagPart setting

IMHO select content types only for given single stereotype per bag part would ok

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jul 16, 2022

Here is a visual of how the settings page looks like
bagpart settings

@ns8482e
Copy link
Contributor

ns8482e commented Jul 17, 2022

If you you need to use widget as contained type then why not you use Flow?

@ns8482e
Copy link
Contributor

ns8482e commented Jul 17, 2022

IMO collection editor is designed only for specific stereo type and each editor is designed to work with specific stereotypes.

It’s better to create new collection part and editor to for different stereotype

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jul 17, 2022

@ns8482e this is for a custom Stereotype not widgets. Why would we over complicate things and and create new part for each Stereotype where we can easily make BagPart more flexible? FlowPart is designed specific to render Widgets with a specific look and feel. Stereotype is just a way to group content types that share a common functionality. By adding a way to provide the Stereotype, we are simply including a group of content-types. At the end of the day, the BagPart will render a collection of content-types as it currently does today but without to manually have to manage the contained content types manually.

I think the screenshot the example in the screenshot I put earlier caused a confusing. I just updated the screenshots above with a proper example.

@ns8482e
Copy link
Contributor

ns8482e commented Jul 17, 2022

May be you are misunderstood Stereotypes, they are not for grouping content types, they provide different functions for different purpose

  • Menu stereotypes provide menus and navigation
  • Widget stereotype provides Widget on layers, flows and widget part
  • Content stereotype(default) provides content items, Bag, ListPart are container for this kind of stereotype.

If you are adding custom stereotype for different functions then it’s better to provide different custom container type for your custom stereotype.

@MikeAlhayek
Copy link
Member Author

@ns8482e yes I think I understand Stereotypes. I stated that in my last comment. I have multiple content types that share/provide the same function. For that reason, I created Stereotype for these content types. But instead of having to manually select these content type in the BagPartSettings, I would provide the Stereotype in the BagPartSettings and the BagPartDiaplayDriver will then render all of the content type that has the same Stereotype.

@Skrypt
Copy link
Contributor

Skrypt commented Jul 19, 2022

What I suggested to @CrestApps in a Gitter discussion is that we should probably think about Content Type filtering like we did with tenants. Filtering could be done also by stereotype because naturally, it becomes a way to group Content Types. Though, it would be better to have simply grouping for Content Types at that point. But, then the changes proposed here makes sense for StereoTypes only and the next natural step is to provide grouping for Content Types and to change the actual Content Type selector shape accordingly by providing the different options (select by type, by group, or by stereotype).

Here, I'm not against the proposal but it should maybe cover more than only one use case which is "stereotypes" here in that case.

And yes @ns8482e I kind of agree with you that StereoTypes have not been made for grouping. But they are actually a way to do this too. If we had a way to group Content Types then we would probably not really have a discussion about StereoTypes.

@MikeAlhayek
Copy link
Member Author

@Skrypt yes grouping make sense here as discussed. But I think that is a larger scope which can be added as a continuous PR to #11978.

I don’t think there is a good reason not to add Stereotype support. At the end of day, it just a way to select multiple types that provide common/similar functionality. Also, this extends existing functionality and has no breaking change. If this PR is accepted, I’ll carve out time to create a continuous PR to add actual grouping support.

@ns8482e
Copy link
Contributor

ns8482e commented Jul 19, 2022

@Skrypt yes can be repurposed for grouping but I don’t think that is good idea,

I see stereotype as an abstract and each stereotype implementation has its own family and purpose and we should discourage ( not against) using BagPart for mix n match

Think of a future editor eg Gutenberg editor where you might use content type with Gutenberg stereotype that only work with Gutenberg editor (like widget works with flow) and with this change BagPart will be broken and has no idea to handle such unknown stereotypes.

Current purpose of BagPart is to work with content if grouping is the ask then we should add tags/categories in content type definitions and filter using them instead of using stereotype for that purpose

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jul 19, 2022

@ns8482e it is not mix and match. It’s either picking content-types OR specifying stereotype.

How would adding Gutenberg break the BagPart in your example? If you created a new stereotype and created all the needed templates for it everything should work just fine.

The only time we use the Stereotype provided in the BagPart settings is to do this

contentTypes = _contentDefinitionManager
                             .ListTypeDefinitions()
                             .Where(contentType => contentType.HasStereotype() && settings.ContainedStereotypes.Contains(contentType.GetStereotype(), StringComparer.OrdinalIgnoreCase));

We only use the Steryotype to find all content types that share the same Stereotype (abstraction as how you think about it). Now sure what I am missing

@ns8482e
Copy link
Contributor

ns8482e commented Jul 19, 2022

Even in case of either - BagPart can’t handle editing of unknown template of unknown stereotype so in that case the proposed implementation be broken,

To me it’s like use on your own risk 🤔 If that’s the case then the proposed implementation should not be ootb

@ns8482e
Copy link
Contributor

ns8482e commented Jul 19, 2022

How would adding Gutenberg break the BagPart in your example?

It’s just an example - its unknown template - I feel like your current assumption is that unknown template will always be compatible with BagPart editor that might not be the case

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jul 19, 2022

How is that any different that attaching BagPart to a content type but not set ContainedContentTypes. Or, setting ContainedContentTypes to a specific content-type and then delete the content-type from the content type definition.

The risk of an admin doing something wrong is there "all over" not just in BagPart. I think this enhancement help users that already use custom Stereotype for their needs. It'll enable identifying these content types by their known Stereotype as per the custom design. I personally encountered this need in couple projects

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