Navigation Menu

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

Added content apps for document types #8187

Merged
merged 4 commits into from Jun 19, 2020
Merged

Added content apps for document types #8187

merged 4 commits into from Jun 19, 2020

Conversation

patrickdemooij9
Copy link
Contributor

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
docTypeWithCustomApp

Doc type with 2 custom content apps (1x C# and 1x manifest)
docTypeWithCustomApps

@ronaldbarendse
Copy link
Contributor

Another use-case for a content app: configuring default values (#7859) 👍

@nul800sebastiaan
Copy link
Member

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! 👍

Copy link
Contributor

@ronaldbarendse ronaldbarendse left a 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!

@nul800sebastiaan
Copy link
Member

Thanks for the feedback @ronaldbarendse and the speedy fixes after feedback @patrickdemooij9 - good stuff. Let's get this tested and merged! 👍

Copy link
Contributor

@poornimanayar poornimanayar left a 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

@poornimanayar poornimanayar merged commit 591575b into umbraco:v8/contrib Jun 19, 2020
@poornimanayar
Copy link
Contributor

And merged :-) Thanks for your work ⭐

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

5 participants