-
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
V15: Document Type Create Options #17669
Conversation
To extend, rather than reinvent the wheel.
…item` instead of `uui-ref-node`, so we can use `umb-icon` (with color support) and UI consistency.
Deprecates `umb-document-type-create-options-modal` and token.
- Document Type (default) - Document Type with Template - Element Type - Folder
if (this.icon) { | ||
const umbIcon = document.createElement('umb-icon'); | ||
umbIcon.setAttribute('name', this.icon); | ||
this.shadowRoot?.querySelector('#icon')?.replaceWith(umbIcon); |
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 think this is a little dangerous,, cause what if something caused Lit to re-render.. then it would not be overriden any more..
Could you try to just add it as a direct child, not in the shadowRoot and then set the attribute slot='icon'
?
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.
It felt a bit hacky when I was developing that part. I did try a few other approaches, but I couldn't get the icon to display, the fallback icon kept being rendered.
For reference: https://github.com/umbraco/Umbraco.UI/blob/v1.12.2/packages/uui-ref-node/lib/uui-ref-node.element.ts#L109-L112
When you say "add it as a direct child", do you mean that it overrides the render()
method? If so, I'm not quite sure how that would work.
I'm wondering if UUIRefNodeElement
could have a protected renderIcon()
method added to it, that may simplify this. 🤔
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 have changed this to append a child of it self. This can later be updated to override a method.
|
||
@property({ type: String }) | ||
detail = ''; | ||
const elementName = 'umb-ref-item'; |
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 think we agreed not to do this any longer? :-D
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 thought we agreed the opposite, that we would do this. 😂 I'm cool with reverting this, just let me know.
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.
yeah, well the idea is to get rid of it in time. so I would say yes, lets not add it in this PR. :-)
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 have corrected this
Hi @leekelleher really great refactoring, that is so nice to get aligned and make extendable. so thanks for that. I just skimmed the code of interest, so not because I'm reviewing per say, but I had two minor comments :-) |
Hi @leekelleher , It's fantastic to see that you have implemented the create options for Document Types. This is one of the more interesting cases! :D Based on the current state of your PR, I would appreciate it if we could address these areas to ensure maximum flexibility for extensions and at the same time decouple our own code more. I suggest we move all template logic from the Document Type into the "Template"-module. This means that if the template package is not installed, all related logic around Document Types with templates would be removed (headless cms):
As we work on these changes, we should also consider how to implement a solution that can be reused across all workspaces. We already have a snippet/preset functionality for Partials Views which could utilize the same system. |
import type { MetaEntityCreateOptionAction } from '@umbraco-cms/backoffice/entity-create-option-action'; | ||
|
||
export class UmbDefaultDocumentTypeCreateOptionAction extends UmbEntityCreateOptionActionBase<MetaEntityCreateOptionAction> { | ||
override async execute() { |
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.
Since we are generating a HREF/URL, then we should not use an exeucte method, but instead use the getHref method. so the actual URL is exposed to the user and the user can use such to open in a new tab etc. More transparent and accessible. I have corrected this.
override async execute() { | ||
if (!this.args.entityType) throw new Error('Entity type is required to create a folder'); | ||
|
||
const createFolderAction = new UmbCreateFolderEntityAction(this, { |
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 have also changed this to use inheritance instead
I have been through this code changes and a bit related code to correct a few things. I have added comments on each change so its clear and we can discuss if necessary. Feel free to let me know what you think, and then I hope this will help move this PR forward :-) @madsrasmussen do you have some structural changes you like to take part of this PR. I like your idea but I'm unsure if it fits within this PR. Your call, I'm open for the inputs :-) |
FYI: I have removed the custom headline option for the create dialog. I would like to solve this in a more generic way so the dialog will handle it. It could be showing a breadcrumb of where something will be created. We will have to live with a general "Create" text for now. I hope it makes sense. |
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 Hi,
I think we have all been over this now, so let's get it merged 😄
- Everything works as expected 👍
- We have removed code duplication by using an extension point 👍
My dream for code splitting and extendable presets will be for another PR.
Awesome work 💯
Description
Creates an
entityCreateOptionAction
extension for each of the Document Type create actions. This builds on the work started in umbraco/Umbraco.CMS.Backoffice#2509, and replaces the existing "Document Type Create Options modal" (which has hardcoded options).To align the UI/UX consistency for the existing Create Options, amends were required to the
<umb-ref-item>
component, (refactored to extenduui-ref-node
rather than duplicate code/functionality). This mostly meant updating existing usage to add theselect-only
attribute.// @madsrasmussen Tagging you in, as you may wish to cast an eye on these amends.
How to test?
Go to the Settings section, try creating a new document type: