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

Comments

@BobbieBarker

BobbieBarker commented Sep 15, 2015

Recreate BS4 modal with ng2 component.

@BobbieBarker BobbieBarker referenced this issue Sep 15, 2015

Closed

Initial Push for Core Boostrap 4 Components #1

16 of 17 tasks complete

@wesleycho wesleycho modified the milestone: 0.1.0 (Preview) Sep 15, 2015

@wesleycho wesleycho removed the help wanted label Sep 15, 2015

@wesleycho wesleycho self-assigned this Sep 16, 2015

@wesleycho

This comment has been minimized.

Member

wesleycho commented Sep 24, 2015

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

@wesleycho wesleycho removed the PRs plz! label Sep 24, 2015

@pkozlowski-opensource

This comment has been minimized.

Member

pkozlowski-opensource commented Apr 13, 2016

@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

This comment has been minimized.

Member

Foxandxss commented Apr 13, 2016

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

This comment has been minimized.

Member

pkozlowski-opensource commented Apr 13, 2016

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

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

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Apr 13, 2016

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

@wesleycho

This comment has been minimized.

Member

wesleycho commented Apr 13, 2016

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

This comment has been minimized.

Member

pkozlowski-opensource commented Apr 13, 2016

@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

This comment has been minimized.

Member

pkozlowski-opensource commented Apr 14, 2016

@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

This comment has been minimized.

Member

Foxandxss commented Apr 14, 2016

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

This comment has been minimized.

Member

pkozlowski-opensource commented Apr 14, 2016

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

@pkozlowski-opensource

This comment has been minimized.

Member

pkozlowski-opensource commented Jun 20, 2016

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

@natcohen

This comment has been minimized.

natcohen commented Jul 11, 2016

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

@wesleycho

This comment has been minimized.

Member

wesleycho commented Aug 19, 2016

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

This comment has been minimized.

alvipeo commented Aug 19, 2016

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

@wesleycho

This comment has been minimized.

Member

wesleycho commented Aug 19, 2016

@pkozlowski-opensource is on the Angular team :)

@alvipeo

This comment has been minimized.

alvipeo commented Aug 19, 2016

:) ok

@jnizet

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Member

wesleycho commented Aug 19, 2016

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

This comment has been minimized.

Member

pkozlowski-opensource 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.

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

iBasit commented Aug 21, 2016

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

@alvipeo

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

pkozlowski-opensource commented Aug 24, 2016

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

This comment has been minimized.

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

This comment has been minimized.

Member

wesleycho commented Aug 24, 2016

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.

pkozlowski-opensource added a commit to pkozlowski-opensource/core that referenced this issue Aug 24, 2016

@fxck

This comment has been minimized.

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.

pkozlowski-opensource added a commit to pkozlowski-opensource/core that referenced this issue Aug 25, 2016

pkozlowski-opensource added a commit to pkozlowski-opensource/core that referenced this issue Aug 26, 2016

pkozlowski-opensource added a commit to pkozlowski-opensource/core that referenced this issue Aug 26, 2016

pkozlowski-opensource added a commit to pkozlowski-opensource/core that referenced this issue Aug 26, 2016

pkozlowski-opensource added a commit to pkozlowski-opensource/core that referenced this issue Aug 29, 2016

@felixfbecker

This comment has been minimized.

felixfbecker commented Sep 18, 2016

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

This comment has been minimized.

akram1905 commented Oct 6, 2016

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

Merge pull request ng-bootstrap#3 in UMAMI/ng-bootstrap from jm_dropd…
…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