feat(dialog): rewrite $dialog

[ProLoser]

I think that at this point it is cleat to everyone that the current $dialog service got in a state where it is hardy maintainable. There are tons of issues opened for it and some of the design decisions make it impossible to move forward with the current code.

I'm in the process of re-writing the current service and the modal directive, will try to push the first version for review over the weekend (if everything goes as planned).

Going to close all the existing issues for $dialog and 'modal' as a duplicate of this one.

As a reminder, early draft of the design is available here:
https://gist.github.com/ajoslin/5477996

@ajoslin I'm not sure if keeping is as gist is the best thing at the moment as I can't easily modify it. Not sure how to progress - I can fork of course but how do we merge all the versions? Anyway I will be working on the forked gist for now.

[kadamwhite]

Interested to see where this goes, I just ran across #232 myself. Good luck!

[fredsa]

The proposal writes """keyboard - Indicates whether the dialog should be closable by hitting the ESC key, defaults to true""". But keyboard should go further, e.g. provide a way to have a default button focused (issue 144), or provide keyboard shortcuts / key bindings for buttons.

[Narretz]

How's it going you guys? Any fork already on github that can be checked out / can be helped with? One thing that is dear to me is good support for using one dialog with different controllers, e.g. the re-opening of a currently open dialog with new a controller. I used it for a Login dialog that seamlessly becomes a register dialog.

[pkozlowski-opensource]

@Narretz Still wip, hopefully I will push a new branch in a week or so. Regarding your use case - I'm not sure I follow - are you after re-using the same partial with different controllers?

[Narretz]

Yep, exactly, use the same dialog with different controllers (templates, resolves). At the moment, this works nicely with re-opening an existing dialog with a new config object. Otherwise you'll get some flicker when you close the first and open the second dialog.

[pkozlowski-opensource]

@Narretz yeh, so in the re-written version we are going to dump distinction between opening a pre-configured dialog and opening a dialog. From where the observed flicker is coming? Is it the resolve section that eats up time?

[adam77]

Great to see the dialogs getting some love. Hope to see a preview soon...

[Narretz]

Hm, I made a plunkr to show the re-opening issue.
In the first dialog, you can switch to the next dialog, first by re-opening an existing dialog, and second by closing the existing and opening another one. In the second case, you can see a notable flicker, because the dialog needs to be removed from the dom, and then added again. There's no flicker in the first case, as the dialog stays the same, only the content changes. There's problem in the first case though that the backdrop isn't removed after closing the re-opened dialog.

http://plnkr.co/edit/MSNtSz6Nr2HA8CMaeMzZ?p=preview

[scamden]

@hall5714 haha ok now you have me really thinking i'm doing stuff wrong. so i'm using the tpls version because otherwise the partials wouldn't load... aren't the templates meant to be used? how else are the components supposed to work? are you suggesting that i write my own version of all the partials in the library or copy the templates i want to use to the appropriate path? that seems messy.. i'm installed via bower btw, although i also have the source

[hallister]

@scamden I'll try to give you a basic run down 😄 The template version compiles all the HTML into javascript strings and stores them in $templateCache. This way when it's used in production we aren't constantly sending HTTP requests for template partials. So you have two ways you can do this:

Use the source and individually include components (literally have <script></script> tags for each directive you use individually). Or use the non template version of the minified source and make sure the entire template directory is in your path. The former has the advantage of being able to run the normal grunt job to compile the production version of ui-bootstrap, but with your custom templates.

In either case you can modify the templates for your particular use-case. I prefer the former, but that's largely because bytes are expensive with large proejcts, and I want to ensure I include nothing more than I need for any given project.

[mfrye]

I've been going through the code and it definitely looks a lot cleaner. I'm having some trouble showing the modal though. It's 2 things... 1) It looks like the Bootstrap code display: none isn't switching to display: block, although I see in the modal code it's supposed to -> self.modalEl.css('display', 'block'), and 2) For some reason scope.animate isn't being set to true to trigger the opacity fade.

Am I doing something wrong, or is it a bug in my app code? Here's how I'm triggering the modal.

scope.openWidgetModal = function() {     
        var modalOptions = {
          template: '<div class="modal-dialog"><widget-inner-tabbed ></widget-inner-tabbed></div>'
        };
        $modal.open(modalOptions);
}

[Narretz]

Does the modal only work with angular >= 1.2? I have the same problem as mfrye

I can't find any reference to "display : block" in the code. I guess it's handled purely by animations. Even if you don't have animations, you have to set the display of the modal to "block" in your css, and remove all bootstrap styles that are introduced by the "fade" class in the modal window directive.

    .modal {
        opacity: 1;
        transition:;  none;
        display: block;
    }

    .modal.fade .modal-dialog {
        transform: none;
        transition: none;
    }

This opens the modal without any animation at all.

[pkozlowski-opensource]

@Narretz @mfrye no, it should work perfectly fine with AngularJS >= 1.0.5. The problem seems to be in the unneeded <div class="modal-dialog"> div, IMO. But hard to say without a live reproduce scenario. If one of you guys could send a plunk it would be awesome.

I'm going to merge this refactoring PR during the weekend as IMO all is done apart from docs. So spotting any bugs early on would help making sure that we've got a solid service here.

[albv]

Hi, I have the same issues as @Narretz and @mfrye. I think that it's because using Bootstrap 3 styles. Class .modal-dialog comes from there actually. I could create plunk, but I thought that this version is not intended to work with Bootstrap 3 yet. Am I wrong?

[pkozlowski-opensource]

@albv ah, this would explain things! No, this version is meant to be used with BS 2.3.x. I'm going to merge it over the weekend, re-base the bootstrap3 branch on top of the latest master and then we will be able to change it to support bootstrap3.

[Narretz]

I think it has nothing to do with bootstrap.

The problem is that the "in" class for the modal never gets set:

http://plnkr.co/edit/YvDS917yLqqmIoqD2Zk9?p=preview

In the modal-window directive, $timeout is used to set $scope.animate = true. But it appears that the last argument of timeout (the invokeApply) skips the apply, so the change in $scope.animate is never picked up by the view layer. Or something like that. Line 179 in bootstrap.js in the plunker.

    //trigger CSS transitions
    $timeout(function () {
      scope.animate = true;
    }, 0, false);
    //$timeout does not trigger apply

It does work when set to true

[mfrye]

@pkozlowski-opensource yeah I wasn't sure if you had integrated BS 3 or not. They made a couple changes in BS 3 to where the standard modal is:

<div class="modal">
    <div class="modal-dialog">
    The modal well / widget content...
    </div>
</div>

// Where the modal dialog is centered in the modal with modal css as...

.modal {
position: fixed;
top: 0;
right: 0;
bottom: 0;
left: 0;
display: none;
}

BS2 had the modal in the center rather than taking up the whole screen, and didn't have the display: none as well. So I believe the modal class would now need to be clicked instead of the modal-backdrop to hide everything.

I don't believe I can create a plunker to replicate the exact setup I have as the modal template I'm using is apart of a fairly complex component setup.

[scamden]

@hall5714 thanks for the run down! i was aware of the way the template cache / template module builder thing works cause we stole the idea from this project to use in our build! sweet for testing.

anyway to your suggestion, three questions:

1. is it really a good idea to modify the templates? what if there are changes? i have to carefully merge the changes and then remodify the templates? i'm pretty new to the open source thing but i was kinda working on the assumption that i should be able to use the library without modifying it and submit pull requests for modifications (although i've already broken that rule to get functionality that i had to have, but i felt dirty doing it).
2. in the former scenario you mentioned, it would seem i still have to copy the templates i want to use or modify into the proper folder on my path (out of the installed bower folder) which seems to me to defeat the purpose of installing with bower. if i leave them in the bower folder, the templateUrls are all off. or maybe you could explain a little further what you meant by using the normal grunt process?
3. i don't think modifying the templates will be enough to get a transparent backdrop in one case but the normal one in another. with modal window i can just remove the class attribute on the container since it's transcluded and put my style on the content. but the backdrop doesn't get modified at all and has isolate scope. any thoughts on how i could dynamically set a class on it?

it seems to me that allowing us to supply the templates to the modal service would fix all of these issues, and not require me to modify the library, but i'm definitely open to other suggestions. thanks again for helping a fledgling open sourcer.

[hallister]

@scamden No problem :)

Ideally modifying the templates IS the way we can customize this stuff. In my perfect universe each and every directive would be completely independent of styling altogether. So you could drop in Foundation/Bootstrap 2 or 3/Whatever and add some ng-stuff and you are good to go. The idea here is that templates are where the magic happens, the directives are just "dumb functions" so to speak. You can certainly use this library without modifying anything, but the idea of templates is that they give you the ability to overwrite the styling without making modifications to the core directives.

And my method does indeed make bower all but useless. I typically don't use bower often, because where I don't modify I typically use CDNs, and where I do modify I typically want customization (Bootstrap, Angular-UI, etc). As for the grunt process, that was a bit off. I should have said build process... basically building the source via grunt and creating your own dist files.

On your last point, at this moment there isn't a way to define custom backdrop/window elements. Basically backdrop can be "turned off" but when on it always compiles a <modal-backdrop> element. Not sure if/when this will be configurable but I imagine that's a definite possibility :).

[scamden]

thanks @hall5714 ! i think i have my head wrapped arounda decent build now. really appreciate the tips.

unfortunately even modifying the window.html template isn't going to work. the tests rely on the modal class being there in order to find them in the dom.. it seems i can't remove that class and still be able to build. is this refactor meant to replace the $dialog service because it seems very opinionated that it's only for modals?

[cesarp]

I had to change the modalWindow and modalBackdrop link function timeout to be able to show the modal otherwise the HTML is created but the in class is never added to the modal so it stays hidden.

$timeout(function () {
                    scope.animate = true;
                }, 0, true); // use true instead of false

Also for IE8 I had to change the $modalStack.open $compile calls to

var modalDomEl = $compile(angular.element('<div modal-window>').html(modal.content))(modal.scope);

and

backdropDomEl = $compile(angular.element('<div modal-backdrop>'))($rootScope);

because using <modal-window> and <modal-backdrop> throws an error.

Thanks for your hard work 👍

[pkozlowski-opensource]

Guys, the rewrite just landed in master as d7a4852!

Thanks to all who took time to review the early draft, especially @Narretz and @scamden and @hall5714

I'm going to close this PR now (plus some other associated issues). I need to re-read through the whole conversation regarding class overriding but @hall5714 is right, the way to go is to override templates. @scamden to override a couple of templates you don't need to build anything, just use one of techniques described here:
http://stackoverflow.com/a/17677437/1418796

If there are still unresolved problems let's open separate issues for each problem.

One again, thnx for your patience and early feedback!

[scamden]

@pkozlowski-opensource thanks! that stack overflow totally fixed the modal window side! thanks so much! still would be nice to be able to have differing backdrops, but this crushes so many bugs with the old service that i can't really complain :)

[stivni]

@pkozlowski-opensource when will this fix be available in a release in the bower repository? Thanks

[sianabanana]

Can i ask, what is meant to go in the window.html and backdrop.html file?

the demo you have included doesn't have these files and the demo doesn't seem to do anything? so im finding it a bit hard to work out how to actually use the new modal service.

[pkozlowski-opensource]

@stivni We should cut a release this weekend - can't promise but this is the plan for now.

[pkozlowski-opensource]

@sianabanana those are default templates for backdrop's HTML and modal window decoration HTML. If you want to use standard bootstrap look & feel you don't need to change those nor include this markup in your modal's content.

[richardcrichardc]

Are there any plans for a replacement for $dialog.messageBox in any shape or form?

[richardcrichardc]

I've committed to using the new $modal, but was relying on $dialog.messageBox. So I've knocked together a replacement based on the original. It's not pretty and is undocumented, but it does the job.

Here it is for anyone in the same boat (note the code has been updated on 9-Sept-2013 with asafdav's feebdack):

angular.module('messageBox', [])

.controller('MessageBoxController', ['$scope', '$modalInstance', 'model', function($scope, $modalInstance, model){
  $scope.title = model.title;
  $scope.message = model.message;
  $scope.buttons = model.buttons;
  $scope.close = function(res){
    $modalInstance.close(res);
  };
}])

.factory('messageBox', ['$modal', function($modal) {
  return {
    open: function(title, message, buttons, modalOptions){
        var options = {
          templateUrl: 'templates/messageBox/messageBox.html', 
          controller: 'MessageBoxController', 
          resolve:{
            model: function() {
              return {
                title: title,
                message: message,
                buttons: buttons
              };
            }
          }
        }

        if (modalOptions)
          angular.extend(options, modalOptions);

        return $modal.open(options);
      }
  }  
}]);

The template which should go in 'templates/messageBox/messageBox.html' is just a copy of templates/dialog/message.html:

<div class="modal-header">
    <h3>{{ title }}</h3>
</div>
<div class="modal-body">
    <p>{{ message }}</p>
</div>
<div class="modal-footer">
    <button ng-repeat="btn in buttons" ng-click="close(btn.result)" class="btn" ng-class="btn.cssClass">{{ btn.label }}</button>
</div>

After including the module and having messageBox injected you can call messageBox.open(). Open takes the same three args as messageBox in $dialog (see https://github.com/angular-ui/bootstrap/tree/0.5.0/src/dialog), the is an extra optional argument modalOptions that can be used to add options to be passed to $modal.open().

[asafdav]

richardcrichardc thank you for sharing your messageBox code,
You've forgotten to return the modal instance in your "open" method, I think it makes it much more usable.

Just replace

        if (modalOptions)
          angular.extend(options, modalOptions);

        $modal.open(options);

With

        if (modalOptions)
          angular.extend(options, modalOptions);

        return $modal.open(options);

Then you can use it like regular $modal, for instance:

var modal = $messageBox.open(title, msg, btns);
modal.result.then(function(result){
  if (result === "ok") {
    console.log("great");
  }
});

Thanks again

[richardcrichardc]

asafdav - very good point. I've updated the code in my previous comment.

[georgiosd]

Is there any peculiar reason why an ng-include would not work inside a modal?
The funny thing is that the URL is retrieved, angular generates the <!-- ngInclude url --> comment as normal, but the content is not there!
Any ideas?

[chrislambe]

@georgiosd I'm having the same issue; did you ever manage to get an ngInclude directive working inside a $modal instance template?

[georgiosd]

I honestly cannot remember and i am away from my computer for a month :(