Umbraco.CMS.Backoffice icon indicating copy to clipboard operation
Umbraco.CMS.Backoffice copied to clipboard

Feature/Proposal: Added support for element factory for modal manager context

Open enkelmedia opened this issue 10 months ago • 2 comments

This addresses https://github.com/umbraco/Umbraco-CMS/discussions/16677 to be able to support custom "modal host" elements. That is not the content of the modal but the modal it self. I figured I'll implement something to show how I think it could be done.

I also added a working example in the /examples/ folder.

Description

This PR will make it possible for extensions (and the core) to extend UUIModalElement to create a custom modal element.

This is made possible by the added elementFactory parameter that is called by modal.element when the modal is created.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

This makes it possible to extensions and core developers to create other types of modals while still reusing the foundation with the core Modal Manager Context, routing etc.

How to test?

There is a example included in the PR, to try it out you would need to find a way to open the modal.

In some element add code like this:

#modalContext?: UmbModalManagerContext;

constructor() {
	super();

	this.consumeContext(UMB_MODAL_MANAGER_CONTEXT,(instance)=> {
	      this.#modalContext = instance;
        });
}

onClickModal(){

	const modalContext = this.#modalContext?.open(this, EXAMPLE_MODAL_TOKEN, { value : {
		text : 'modal text'
	}});

	modalContext
		?.onSubmit()
		.then((e) => {
			console.log('Modal returened:', e.text)
		})
		.catch(() => {
			console.log('Modal rejected')
		});
}

Let me know what you think and if there is any adjustments you would like me to make.

Cheers!

enkelmedia avatar Mar 31 '24 02:03 enkelmedia

Hi @enkelmedia

Thanks for this concept, it might be interesting for us as well when we start to looking into Tours.

Only thing that strike me is that we in general dont call things Factory. And actually having a prop that accepts either a element-name or a method that returns an element is something we have in a few places. (I know that this one does not accept a element-name of now, but I think it would make sense that it could) And in these place we do not use this term, so could we remove factory and just call it element or if that potentially becomes confusing then modalElement?

How does that sound to you :-)

nielslyngsoe avatar Apr 02 '24 19:04 nielslyngsoe

Thanks for the feedback! Glad you found it interesting.

Great point with the factory part, I did not put a lot of effort in the naming.

I was thinking about this now and thought that maybe this method could be confused with the way that the "mounted view" inside the modal is configured (I mean in the manifest for of type 'modal'). But then I thought that in the context of setting up the token (configure type, size etc.) makes it quite clear that we're configuring the modal it self and not the "mounted element". So given that I'm quite happy with just element and then make sure to add comments/docs to clarify.

About the way to "point out" the element I guess that it would be nice if this was done in the same way as other elements, are we thinking about the element: ()=>import('./yada-yada.js')-approach? I guess it would be possible to set constrains so that only a element that extends UUIModalElement can be used? Not sure how.

Something also need to create the child uui-dialog, this done in the "factory" right now, I guess we could move that into the #createContainerElement() method of modal.element.ts?

Do you happen to have any pointers as to were to look for examples of how this is done in other places?

Happy to update the PR, rename the method and adjust how the element is configured.

enkelmedia avatar Apr 02 '24 20:04 enkelmedia

Hello @enkelmedia Would you be willing to adjust this PR with the proposed change in https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1507#issuecomment-2032889983?

iOvergaard avatar Oct 14 '24 13:10 iOvergaard

@iOvergaard Will do =D

enkelmedia avatar Oct 14 '24 13:10 enkelmedia

I've been looking at this and kind of not sure which direction to take this.

Since @nielslyngsoe requested the elementFactory-method to be able to

having a prop that accepts either a element-name or a method that returns an element is something we have in a few places

I started to go down the path of using ElementLoaderProperty<UUIModalElement> and then loadManifestElement. However, since loadManifestElement is async I'm not sure where to put the loading-logic.

I ended up adjusting the UmbModalElement, making createModalElement async and require backoffice-modal-container to call this method when creating a new instance of UmbModalElement.

I'm not sure if this was the kind of change the @nielslyngsoe was requesting or if he just referred to an actual element-instance or a string-element name? If we don't need the element: ()=>import('./yada-yada.js') then registering the modal - we could avoid all the async stuff.

I might also lean towards naming the property modalElement, feels like that is more clear - let me know what you think.

I'll push the updates here for you to review.

enkelmedia avatar Oct 14 '24 15:10 enkelmedia

I think this is the way to go by making it async, @enkelmedia. I think this is what Niels meant, as that matches other areas of the Backoffice.

"Element" is what we call properties like this in other areas of the Backoffice as well.

I stumbled upon the modal type, you added (custom) and thought that's not as generic as we potentially could make it. I think declaring a global interface thing that you can extend here would be more "correct", especially considering the work we have been conducting on making manifest types being declared globally. I would be interested in hearing @nielslyngsoe's opinion on that :-)

iOvergaard avatar Oct 16 '24 12:10 iOvergaard

@iOvergaard, nice that you think I made the right choice :)

I'm happy to make adjustments based on any feedback from @nielslyngsoe - just let me know :)

// m

enkelmedia avatar Oct 19 '24 13:10 enkelmedia