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

Feature: Custom modal type #1507

Merged
merged 11 commits into from
Nov 7, 2024
Merged

Conversation

enkelmedia
Copy link
Contributor

@enkelmedia enkelmedia commented Mar 31, 2024

This addresses umbraco/Umbraco-CMS#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)
  • 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!

@nielslyngsoe
Copy link
Member

nielslyngsoe commented Apr 2, 2024

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 :-)

@enkelmedia
Copy link
Contributor Author

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.

@iOvergaard
Copy link
Collaborator

Hello @enkelmedia
Would you be willing to adjust this PR with the proposed change in #1507 (comment)?

@enkelmedia
Copy link
Contributor Author

@iOvergaard Will do =D

@enkelmedia
Copy link
Contributor Author

enkelmedia commented Oct 14, 2024

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.

@iOvergaard
Copy link
Collaborator

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 :-)

@enkelmedia
Copy link
Contributor Author

@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

@nielslyngsoe nielslyngsoe self-assigned this Nov 7, 2024
Copy link
Member

@nielslyngsoe nielslyngsoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@nielslyngsoe nielslyngsoe changed the title Feature/Proposal: Added support for element factory for modal manager context Feature: Added support for element factory for modal manager context Nov 7, 2024
@nielslyngsoe nielslyngsoe changed the title Feature: Added support for element factory for modal manager context Feature: Custom modal type Nov 7, 2024
@nielslyngsoe nielslyngsoe merged commit 2273af3 into umbraco:main Nov 7, 2024
8 checks passed
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.

3 participants