-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Added content apps for document types #8187
Added content apps for document types #8187
Conversation
Another use-case for a content app: configuring default values (#7859) 👍 |
Hey there @patrickdemooij9! Sorry it has taken a while for us to reply to this one, I wanted to talk to some people at HQ first before I knew what to say about it. It got surprisingly quiet for a few seconds in that meeting and then people realized how powerful this could be.. So congrats on a great idea! 🎉🎉 The code looks quite nice and clean as well, I haven't done a full review yet, but I understand what you're doing, looks good. There were immediately some ideas flowing for some other apps, like maybe an overview of all the content that is using this content type, or where it is used in compositions or if you're working on your own modelsbuilder implementation for example. Exciting posibilities! We'll get this reviewed soon! 👍 |
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've reviewed the changes and most of it seems good, just some minor white-space issues (mixed tabs/spaces) and usage of angular
helper methods...
One fundamental thing for all IContentAppFactory
implementations though is they throw a NotSupportedException
when the supplied type is not supported, instead of just returning null
. This means all implementations need to be updated whenever content apps are added to a new type. This breaks the open-closed principle, as it's not possible to extend the content apps functionality without modifying these implementations!
src/Umbraco.Web.UI.Client/src/views/documenttypes/edit.controller.js
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/documenttypes/edit.controller.js
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/documenttypes/edit.controller.js
Outdated
Show resolved
Hide resolved
src/Umbraco.Web.UI.Client/src/views/documenttypes/edit.controller.js
Outdated
Show resolved
Hide resolved
Thanks for the feedback @ronaldbarendse and the speedy fixes after feedback @patrickdemooij9 - good stuff. Let's get this tested and merged! 👍 |
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.
Hi @patrickdemooij9,
Fantastic idea! Such a powerful feature which can open up a lot of possibilities as it has been rightly said :) I have tested the PR and it works as advertised. Tested with a package manifest content app as well as C# one with permissions, allowing content apps on specific doc types etc and all such scenarios works! The code looks clean too. I am going to merge the work in. Thanks for your work once again 👍 🥇
Poornima
And merged :-) Thanks for your work ⭐ |
This PR adds the ability to add content apps to document types.
I updated the tabs that are currently used (design, list view, permissions and templates) to be using the new content apps.
Why would you want content apps for document types? I think it's always good to have the possibility to extend the backoffice on all spots, but I actually had an idea I couldn't do because this wasn't possible. I want to be able to set SEO settings for all pages that have a certain document type.
I am not too familiar with the Umbraco code, so I hope everything that I changed is alright! Please let me know if it isn't and I can change it around.
Doc type with 1 custom C# content app
Doc type with 2 custom content apps (1x C# and 1x manifest)