-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
allow users to setup default content type settings visibility #15733
allow users to setup default content type settings visibility #15733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lampersky quick change and I'll merge it.
/// <summary> | ||
/// Configure the default driver options. | ||
/// </summary> | ||
public ContentTypeDefinitionDriverOptions Default { get; set; } = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lampersky please keep this in the driver private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this way, we also have possibility to change default behavior globally, maybe somebody wants to configure it in a different way than all set to true, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. This should be kept private. We don't want anyone outside to change it. The only way to change it is to use the Configure method to configure the options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way to change it is to use the Configure method to configure the options
but it is still configurable via Configure<>()
, so with my change, you could go with code like this:
services.Configure<ContentTypeDefinitionOptions>(options => {
// configure default behavior
options.Default = new ContentTypeDefinitionDriverOptions
{
ShowCreatable = false,
ShowListable = true,
ShowDraftable = false,
ShowVersionable = false,
}
// configure behavior for specific content types...
options.ContentTypes.TryAdd("ContentType1", new ContentTypeDefinitionDriverOptions
{
ShowCreatable = true,
ShowListable = false,
ShowDraftable = true,
ShowVersionable = true,
});
// configure another content type
options.ContentTypes.TryAdd("ContentType2", new ContentTypeDefinitionDriverOptions
{
ShowCreatable = false,
ShowListable = false,
ShowDraftable = true,
ShowVersionable = true,
});
});
I still think this could be useful. If somebody wants to simply hide all those options for every possible content types, he/she can just configure default without re-configuring all existing ContentTypes.
services.Configure<ContentTypeDefinitionOptions>(options => {
// configure default behavior, it will be applied when there is no other configuration provided
options.Default = new ContentTypeDefinitionDriverOptions
{
ShowCreatable = false,
ShowListable = true,
ShowDraftable = false,
ShowVersionable = false,
}
});
maybe it would be good to ask what others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAGNI :)
What if you have multiple projects and each one is modifying this global variable? This is not a good option to expose publicly. There is a reason why I made it private when I introduced this option. Plus, these settings should exists by default on all content-types. These settings are used by OC everywhere and the should almost always exists.
But, feel free to ping others and get more feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you have multiple projects and each one is modifying this global variable?
will happen exactly the same thing as when you will reconfigure same ContentType in many modules, the last will win, anyway I don't want to convince you, I will adjust the code, to your suggestion, and it is not yagni, because I have a real use case that's why suggested this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lampersky thank you. Once you make that change ping me again and I'll merge it.
On the other side, if there is a good case where we need to support this, you can create a separate PR and add more options to handle such a case using ContentTypeDefinitionOptions instead.
For example, add options like
ListableByDefault
CreatableByDefault
....
And set these all to true by default. These would then be used to set the private default options in the driver. But, I would keep that change as part of a separate PR to see if it is something we would take separately. The problem with such approach is that the UI needs these functions. If you set ListableByDefault to false, then any content type that is created by the UI will not be visible on the UI even if you created it by UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lampersky Are you able to make the last updates soon? I would like to merge this soon so it fixes the issue for others. If you like I can hijack the PR and make the last change and then merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it today, I have it on my TODO list.
Since this PR we have lost possibility to configure content type settings from the UI. Now, it is required to setup content type options visibility via code. When you have only a few content types, you can do this, but what if you have 50+ content types?
instead we could allow users to set the default value, and this value will be used for all those contentTypes/stereotypes which are not configured explicitly:
Other than that, configuring those values from the UI was super very useful, I would like to have it back. I understand that hiding is useful for stereotypes, where those settings could be hidden, but for ContentTypes we should still be able to change those options from the UI:
Another idea is to have visibility defaults always set to true, and when you want to hide particular ContentType or Stereotype you have to configure it explicitly to be hidden.
//cc: @MikeAlhayek @hishamco