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

Modal #3

Closed
BobbieBarker opened this issue Sep 15, 2015 · 48 comments
Closed

Modal #3

BobbieBarker opened this issue Sep 15, 2015 · 48 comments

Comments

@BobbieBarker
Copy link

BobbieBarker commented Sep 15, 2015

Recreate BS4 modal with ng2 component.

@wesleycho
Copy link
Member

Some notes:

  • API
    • Investigate observable-based modal animation to allow cancellation (for the future)
    • Suggestion made to be more declarative with a component vs. imperative by passing objects into a service method
      • @wesleycho: Personally, I feel like this would result in an API that is too rigid, and this can be made possible on the user's side with a custom modal component consuming this service. Any points/counterpoints welcome on this

@pkozlowski-opensource
Copy link
Member

@wesleycho are you working / planning to work actively on this one? I've started to look into what is possible so can either take it over or bounce off ideas.

@Foxandxss
Copy link
Member

No one is really having anything assigned. Except the stuff we already have as PR, so feel free on bounce ideas. At the end you are the one who knows more :P

@pkozlowski-opensource
Copy link
Member

At the end you are the one who knows more :P

@Foxandxss this is very dangerous assumption, don't make this mistake :-)

@Foxandxss
Copy link
Member

No, we are counting on that, don't let us down.

@wesleycho
Copy link
Member

I've done looking into this before, but there is an unfortunate thing here - the DynamicComponentLoader kind of sucks to use. It feels like it's underpowered.

We should perhaps talk about what we need the modal to do.

The modal needs to ultimately slot the modal dialog into a container of a user's choice. Angular Material just decided to go the single insertion way (which may be best initially) by forcing the user to use a container element in the template, and then used a service to insert it into the container while storing a ref to the backdrop and modal in the service after inserting via DCL. On destruction, it then gets cleaned up.

I'm not the biggest fan of putting a container right in the template, but it seems to be necessary due to how the DCL works. The DCL seems necessary for this sort of problem, unless we want to go the declarative route and force the user to declare their modal right in the template.

I personally prefer imperative declaration of components such as modals through the usage of a service, but the framework seems to make declarative usages easier. However, if we go the declarative route, we then also run into the same problem as alerts with self destruction & reseting of state.

@pkozlowski-opensource
Copy link
Member

@wesleycho I hear what you are saying. I'm playing with something right now that might work for the "service" usage without too much pain for the end users. Will shore tomorrow or day after tomorrow.

But yeh, DCL needs more love, Misko was showing me some ideas for future evolution. Wish try to push this thing forward.

@wesleycho wesleycho removed their assignment Apr 13, 2016
@pkozlowski-opensource pkozlowski-opensource self-assigned this Apr 14, 2016
@pkozlowski-opensource
Copy link
Member

@wesleycho this is what I've got so far: http://plnkr.co/edit/877XVmEU05icATk3X7ZK?p=preview

Here is the general idea:

  • we've got a container component (ModalDialogContainer) where all the modals (and their backdrops) will be loaded. ModalDialogContainer is loaded dynamically via DynamicComponentLoader::loadAsRoot
  • there are 2 supporting components: ModalBackdrop and ModalDialog (decoration around modal's content). When a modal is opened both are loaded via DynamicComponentLoader::loadNextToLocation
  • finally there is the Modal service with one method - Modal::open(content) where content is a component (for now, it could be a TemplateRef in the future)

Obviously all this requires (much) more work to be usable, I've left some TODOs. There are some open questions as well:

  • what do we do with animations? Nothing? Wait for ng2 solution? Hack something together on our side?
  • do we want to support multiple modals (stack of modals)?

Anyway, here is a starting point, let me know what you think.

@Foxandxss
Copy link
Member

I don't dislike the given solution (I have to check it better, but I have a good first impression).

About your questions:

what do we do with animations? Nothing? Wait for ng2 solution? Hack something together on our side?

I vote to wait. I keep hearing that it is coming and we are actually working on the docs right now, so probably May is a good date for a first version on animations.

do we want to support multiple modals (stack of modals)?

I would prefer not to. People tend to overuse the modals and that only bring pain. Who needs stacked modals anyway?

@pkozlowski-opensource
Copy link
Member

@Foxandxss thnx, I've got exactly the same thoughts regarding animations and stacked modals

@pkozlowski-opensource
Copy link
Member

Blocked on angular/angular#9293 and angular/angular#8617

@pkozlowski-opensource pkozlowski-opensource added this to the beta.0 (Feature-complete) milestone Jun 30, 2016
@natcohen
Copy link

@pkozlowski-opensource is it still blocked? In this thread you say (9th of June) that there are no more blockers...

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Aug 18, 2016

Here is the status update for anyone tracking this issue:

  • There are still some elements missing in the core that would allow us to provide a perfect API. But the good news is that we've found a work-around that will allow us to move forward with a reasonable API. So we are not totally blocked and resuming work here.
  • It looks like supporting multiple (stacked) modals shouldn't be hard at all and we can built in support for this feature from day 0. Still keep in mind that this is bad UX and I'm not sure it makes sense to promote this anti-pattern.
  • I would estimate the current implementation progress for 60% and would expect the initial version to land in few days.

Here is the remaining work if anyone wants to track progress:

  • support for the modal size option
  • support for the keyboard option
  • create type for modal options
  • focus management on modal blur
  • content shift when adding modal-open CSS class
  • support for stacked modals (if we decide to go ahead with this option)
  • change API docs script so we can document services in additions to directives
  • code cleanup

The other good news is that API surface can be much smaller in Angular 2 as we don't need resolve and related options. Here is the gist of the usage with the planned API:

@Component({
  selector: 'ngbd-modal-basic',
  template: `
<template #content let-c="close" let-d="dismiss">
  <div class="modal-header">
    <button type="button" class="close" aria-label="Close" (click)="d('Cross click')">
      <span aria-hidden="true">&times;</span>
    </button>
    <h4 class="modal-title">Modal title</h4>
  </div>
  <div class="modal-body">
    <p>One fine body&hellip;</p>
  </div>
  <div class="modal-footer">
    <button type="button" class="btn btn-secondary" (click)="c('Close click')">Close</button>
  </div>
</template>

<button class="btn btn-lg btn-outline-primary" (click)="open(content)">Launch demo modal</button>
  `
})
export class NgbdModalBasic {
  constructor(private modalService: NgbModal) {}

  open(content) {
    this.modalService.open(content);
  }
}

@pkozlowski-opensource
Copy link
Member

@jnizet
Copy link
Member

jnizet commented Aug 19, 2016

Could you please explain why resolve is not needed anymore? Suppose I want to create a reusable confirmation dialog. I would like to have a confirmationService allowing, from any component, to do for example

delete(foobar) {
  confirmationService.confirm({
    title: 'Please confirm deletion',
    message: 'Are you sure you want to delete this foobar?'
  }).then(() => this.doDelete(foobar));
}

How would I avoid repeating the whole modal template in every component needing that. It seems that if you pass a string and not a TemplateRef, the string is displayed as is, i.e. not compiled.
How would I pass the title, message, and NgbModalRef as inputs to a reusable confirmation component displayed in the modal?

This was trivial to do in ui-bootstrap, because a controller name and a resolve could be passed as argument. And the controller received a reference to the modal instance and the resolved values. But I don't see (probably due to my lack of Angular 2 experience) how to do the same here.

@pkozlowski-opensource
Copy link
Member

Could you please explain why resolve is not needed anymore?

This is the case since in the proposed API you can only open modals with strings / TemplateRef. In Angular2 you can't have a template that is a free-flying string - it needs to be embedded in another template by using a <template> tag. And what is inside the <template> tag captures context of the owning template so you've got access to all the variables of the owning component.

How would I avoid repeating the whole modal template in every component needing that.

Probably by creating a dedicated component. But to be frank I didn't spend much time looking into extensibility scenarios yet - this discussion is a good occasion to do so.

It seems that if you pass a string and not a TemplateRef, the string is displayed as is, i.e. not compile.

Exact. We don't want to "compile" this string since it would require shipping a compiler to a browser and would kill most benefits of the offline compilation. TBH the "string" argument is pretty much useless for anything other than testing.

How would I pass the title, message, and NgbModalRef as inputs to a reusable confirmation component displayed in the modal?

I think that the larger issue is: where would you have a "template" for the common part? Once again, a "free-flying" string won't work in ng2. So we need to think more about this.

One approach to explore would be to allow passing component types as argument to the open method. And in this case we could have something resolve-like (a map of arguments to be passed to the input of a component). In this case an obvious question would be "what do we do with outputs"? Oh, and in any case I'm not a big fan of introducing async resolve since there are so many ways of doing async stuff now (promise, observable, async-await...).

Let's explore things some more but I would like to land the initial version first before spending too much time on extensibility. The good news is that we can always add API - getting taking things out is harder :-)

Thnx for sharing this use-case - this is an important point in the discussion!

@wesleycho
Copy link
Member

We need a way to bind data to our dynamically inserted components I think - is there a mechanism for doing so with ng2 currently?

@alvipeo
Copy link

alvipeo commented Aug 19, 2016

I wonder if angular team is aware of this..?

@wesleycho
Copy link
Member

@pkozlowski-opensource is on the Angular team :)

@alvipeo
Copy link

alvipeo commented Aug 19, 2016

:) ok

@jnizet
Copy link
Member

jnizet commented Aug 19, 2016

I think I would be perfectly fine with a solution where I would do the asynchronous stuff by myself and pass a component type and a map of inputs. This is the kind of stuff I had in mind.

In addition to the data, I think it's important to be able to pass the NgbModalRef to the component, because the component itself must be able to close or dismiss the modal when it needs to.

@wesleycho
Copy link
Member

With this current proposal, it would be possible (but unideal) to do any resolve type behavior by fetching the desired data beforehand in the service wrapper, then store the data on the service, then open the modal which uses a component which also injects the service in order to fetch the data.

Not great, but it would be possible with the current proposal.

@pkozlowski-opensource
Copy link
Member

I think I would be perfectly fine with a solution where I would do the asynchronous stuff by myself and pass a component type and a map of inputs. This is the kind of stuff I had in mind.

Yeh, let's say this is the best proposal we've got for now. But maybe someone can / will come up with alternatives.

In addition to the data, I think it's important to be able to pass the NgbModalRef to the component, because the component itself must be able to close or dismiss the modal when it needs to.

@jnizet NgbModalRef is just an object so I don't see any issue with passing it around.

@jnizet
Copy link
Member

jnizet commented Aug 19, 2016

@wesleycho Yes, that's approximately what I tried, and I managed to do what I wanted. The only remaining annoyances are that I still need to have this in my enclosing component template:

<template #confirmTemplate>
  <confirm-modal-component>
  </confirm-modal-component>
</template>

and that I need to pass this template ref when deleting:

<button (click)="deleteFoo(confirmTemplate)">Delete</button>

Not a big deal, and the stateful service not ideal either, but it works.

@jnizet
Copy link
Member

jnizet commented Aug 19, 2016

@pkozlowski-opensource regarding the NgbModalRef, sure it can be passed around, but since it's what is returned by open(), it's not quite possible to pass it to open(), along with the data and the component type. So open() itself will have to pass it, in addition to the data. I guess we'll just need to impose a naming convention for the input of the component that will receive the NgbModalRef, or that we'll have to pass the name of this input, or something like that.

@jnizet
Copy link
Member

jnizet commented Aug 20, 2016

In case anyone is interested, I managed to create a reusable confirmation service allowing to display, from anywhere, a customizable confirmation dialog, based on the current WIP by @pkozlowski-opensource, and using the idea suggested above by @wesleycho.
It's a bit hackish, but it just requires adding 3 lines of HTML code in the main component template and declaring some directives and providers in the NgModule.

Here's a gist containing the code.

@iBasit
Copy link

iBasit commented Aug 21, 2016

Can't wait this to get release, so I can get rid of angular1 on our project.

@alvipeo
Copy link

alvipeo commented Aug 21, 2016

Yeah, me too. The whole Bootstrap 3 or 4 would be a game changer when it works on angular 2!

@fxck
Copy link

fxck commented Aug 24, 2016

Have you thought about an API where you don't use any services?

It would look like this

<modal 
  #myModal
  [opened]="modalState$ | async"
  (close)="handleClose($event)">

  <p>some content</p>

  <button (click)="myModal.close()">close</button>

</modal>

I think this would be a perfect, stateless component. The only problem is, that you'd need a way to somehow render it at the root component instead of the one you actually put it in, and I'm not aware of a way to do it at the moment.

I have opened an issue about it angular/angular#11020

@wesleycho @pkozlowski-opensource

@pkozlowski-opensource
Copy link
Member

Have you thought about an API where you don't use any services?

Yes, but it is not flexible enough for all use-cases. Ex.: opening a modal window with login prompt from an auth service.

@fxck
Copy link

fxck commented Aug 24, 2016

@pkozlowski-opensource ok, there might be use cases where it would not be amazing, even though one could argue, especially when using something like @ngrx/store or redux(and we all know it's gonna be all the rage) you could have that modal permanently sitting in the root component with

<modal [opened]="!(isAuthenticated$ | async)">

  <login-form></login-form>

</modal>

but I feel that in 95% of other use cases, this way would be far more clear and intuitive than using services, promises and other stuff.

Wouldn't it at least make a sense to be able to use it both ways?

@wesleycho
Copy link
Member

Modals are not something that generally make sense to make declarative - in fact, such attempts generally feels like anti-patterns, since almost all use cases don't fit abstracting it away into components as the main controller of behavior when its state lies above the rest of the UI, as well as creating a dependency on component tree structure unnecessarily when it doesn't fit into that hierarchy at a more DOM oriented perspective.

I'm against componentizing such in an open source library like this as poor API design/bloat, as well as confusing usage due to promoting multiple patterns instead of promoting a solid powerful & extensible use. We already know from UI Bootstrap and countless other UI libraries that promoting multiple patterns for accomplishing the same goal quickly leads to user confusion, which puts pressure on maintainers having to explain to users nuanced views instead of an answer that is straightforward & easily searchable, especially when one approach is fundamentally more sound than another - that is usually a prime indicator of design problems, and it bleeds over to more complex internal code.

Users are free to build abstractions on top of our components to suit their uses, including building component-based modals on top of the library's modal - it makes far less sense to have it at the library level though, since open source libraries should aim to be as extensible & as lightweight as possible.

@fxck
Copy link

fxck commented Aug 24, 2016

...since almost all use cases don't fit abstracting it away into components as the main controller of behavior when its state lies above the rest of the UI...

Not sure if I understood this correctly, but this is not necessarily true. I'm speaking from a redux-and-the-likes perspective. State of most of my modals would be defined on the slice of state the component belongs to.

eg. client add modal, would be inside my clients route component, with state defined in the clients slice of the state, same goes for most of my modals

and when there is a modal that doesn't belong to any particular component, I would put it in the root component.

I don't want to abandon the concept of inputs/outputs, reactive dumb/smart components and go back to promises, ugh. And wouldn't it be more align the html5 dialog element as well? https://developer.mozilla.org/en/docs/Web/HTML/Element/dialog

as well as creating a dependency on component tree structure unnecessarily when it doesn't fit into that hierarchy at a more DOM oriented perspective.

Yes, this is true and it's the only thing bothering me and preventing me from creating the component myself, as positioning of any parent component could screw up positioning of the modal, ideally I'd want the component to be created(or moved to) on the root component(perhaps using some sort of structural directive(?) or maybe something in the decorator, since angular might need to know about it during compilation(??)), and I have opened an issue requesting a feature that would allow it angular/angular#11020

that promoting multiple patterns for accomplishing the same goal quickly leads to user confusion

I get it, I just hoped you guys would have idea how do the "put component somewhere but render it on the root instead", since you were considering this approach.

@felixfbecker
Copy link

One approach to explore would be to allow passing component types as argument to the open method. And in this case we could have something resolve-like (a map of arguments to be passed to the input of a component). In this case an obvious question would be "what do we do with outputs"? Oh, and in any case I'm not a big fan of introducing async resolve since there are so many ways of doing async stuff now (promise, observable, async-await...).

👍

@akram1905
Copy link

Try this css to make stacked modals work:

ngb-modal-window:first-of-type {
    z-index: 1080;
}
ngb-modal-backdrop:first-of-type {
    z-index: 1070;
}

Working for me using version 1.0.0-alpha.5 !
I'm not sure if you can rely on that last opened modal is the first element in DOM!

erictsai6 pushed a commit to erictsai6/ng-bootstrap that referenced this issue Aug 31, 2017
…own-fixes to master

# By jenmak
# Via jenmak
* commit '8712458fa9840b5fe9b495f98ca5c73c71852610':
  Replaces s-dropdown-menu with s-typeahead class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests