custom css class in $modal template

[fxck]

I did read this #441 (comment) and it never really came to a satisfying conclusion.

The thing is, you won't be changing template/modal/window.html 99% of time, what you might need to change quite often is its class.

You might have 3 sets of modals for different cases, all with a different width. And since width is defined on the parent(.modal), how could do it?

The dialogClass option as it was in the old $dialog made imho perfect sense.

[karol-f]

+1

[D3CK3R]

+1

[hallister]

I believe the idea behind removing dialogClass is to make templates "the place" where we do modifications like this. That said, I think this could be done, but perhaps with a modalTemplate or modalTemplateURL. This way we keep dialog free of DOM awareness, and still allow for custom modal classes. @pkozlowski-opensource thoughts?

[fxck]

Yes, that would work pretty well if you defined modal's width in one of the inner elements.. but you don't. And as long as this is supposed to be native javascript plusing for twitter's bootstrap, that $dialogClass is kind of a must.

[karol-f]

@fxck I agree - this is exactly the case.

[hallister]

@fxck and @blnight I'm not understanding why defining the modals width on an inner element would matter in this case. When I say modalTemplate that would actually be the <div class="modal"></div> part. So a $dialog might look like:

$scope.modalOpen = function() {
    $modal.open({
        backdrop: true,
        modalTemplate: '<div class="modal modal-width-override" ng-transclude></div>',
        templateUrl: 'myModalContent.html',
        controller: ModalInstanceCtrl
    });
});

This way we can define separate templates for separate $modal instances. If this wouldn't work, can you give me more details on your use case?

[fxck]

Yes, that's true, but back to what I initally said, you don't usually change anything but the class in the root element. So you'd end up doing this in every initialization.

modalTemplate: '<div class="modal fade one-third" ng-class="{in: animate}" ng-transclude></div>'

modalTemplate: '<div class="modal fade one-half" ng-class="{in: animate}" ng-transclude></div>'

modalTemplate: '<div class="modal fade two-thirds" ng-class="{in: animate}" ng-transclude></div>'

modalTemplate: '<div class="modal fade big-ass" ng-class="{in: animate}" ng-transclude></div>'

So tell me which is the lesser evil? modalClass or modalTemplate ?

[hallister]

@fxck Well, I definately wouldn't do things that way, I think windowTemplateUrl would be the best option. I really dislike class names directives because it crosses the line of the controller defining the view, especially since $modal is actually a service now. Defining class names inside a controller just seems... counter-intuitive to me.

Regardless, this is just my personal opinion. We'll have to wait on Pawel to decide what the right direction is.

[richardcrichardc]

+1 for a way to vary the width of the modal window.

[richardcrichardc]

Here is a patch to add an extra css class to the window div by specifying it as windowClass option when calling open. Tested with regular jQuery and jQueryLite that comes bundled with angular.

--- modal.js
+++ modal.js
@@ -138,7 +138,7 @@
           backdropDomEl = $compile(angular.element('<div modal-backdrop></div>'))($rootScope);
           body.append(backdropDomEl);
         }
-        var modalDomEl = $compile(angular.element('<div modal-window></div>').html(modal.content))(modal.scope);
+        var modalDomEl = $compile(angular.element('<div modal-window></div>').html(modal.content).addClass(modal.windowClass))(modal.scope);
         body.append(modalDomEl);


@@ -266,7 +266,8 @@
                 deferred: modalResultDeferred,
                 content: tplAndVars[0],
                 backdrop: modalOptions.backdrop,
-                keyboard: modalOptions.keyboard
+                keyboard: modalOptions.keyboard,
+                windowClass:modalOptions.windowClass
               });

             }, function resolveError(reason) {

             }, function resolveError(reason) {

[richardcrichardc]

After applying the above patch and:

1. creating a modal with windowClass set, then
2. creating a modal with with windowClass undefined

You will find the second modal has the windowClass of the first modal applied. Oh oh!

One more change is the solution:
@@ -231,7 +231,7 @@
};

             //merge and clean up options
-            modalOptions = angular.extend(defaultOptions, modalOptions);
+            modalOptions = angular.extend(angular.copy(defaultOptions), modalOptions);
             modalOptions.resolve = modalOptions.resolve || {};

This appears to be a bug in the original implementation. A side effect of merging the defaults and specified options is to replace the memorised options with the merged options. FYI @pkozlowski-opensource

[pkozlowski-opensource]

@fxck @hall5714 so, I'm not sure what is the best way of progressing here. On one hand I hate putting css class names in JS code and I agree here with @hall5714 . At the same time this seems to be the easiest option for developers using the library and I'm buying @fxck argumentation here.

So I would say - let's add this option till we can think of a better solution.

@richardcrichardc would you be up to sending a pull request, with docs and test update? If not I'm going to look into this item tomorrow, would definitively like to tackle this one before a next release.

[richardcrichardc]

What repo should I fork?

It might be quicker if you just copy and paste the changes. I wont be offended if you rename the option or anything. I'm not worried about licences - consider it public domain.

Docs:

WindowClass: Add an addition css to the DIV containing the modal. Use this to override the css in the template on a modal by modal basis, e.g. to change the width of the modal.

On 31/08/2013, at 5:04 AM, Pawel Kozlowski notifications@github.com wrote:

@fxck @hall5714 so, I'm not sure what is the best way of progressing here. On one hand I hate putting css class names in JS code and I agree here with @hall5714 . At the same time this seems to be the easiest option for developers using the library and I'm buying @fxck argumentation here.

So I would say - let's add this option till we can think of a better solution.

@richardcrichardc would you be up to sending a pull request, with docs and test update? If not I'm going to look into this item tomorrow, would definitively like to tackle this one before a next release.

—
Reply to this email directly or view it on GitHub.

[hallister]

Sounds good @pkozlowski-opensource. The one brick wall we are going to run into is Bootstrap 3's new modal layout. For example, the width of a modal is now controlled on .modal-dialog. All the border settings are on .modal-content and modal position is controlled on .modal.

So now, do we put {{ modalWindowClass }} on .modal so we can adjust position, or on .modal-dialog so we can adjust width? That's one of the main reasons I'm concerned about sending somethingClass through, because we don't know where the class should be in any given situation.

[jmwolfe]

Sounds like you might need two different class entry points?

[hallister]

@jmwolfe Or three if someone has a use-case that requires the borders to be modified. Which is why I suggested the windowTemplate and windowTemplateUrl. It's not pretty, but it alleviates the framework from having an opinion on what windowClass actually is. Once we start getting windowClass, dialogClass and contentClass as configurable options we have something far worse than a repetitive template.

I really wish I could think of a way to do this that doesn't either require template repetition or class ambiguity, but I haven't yet.

[jmwolfe]

OK, I've been studying the latest module code for a few hours to get the backdrops to stack and block correctly (my Issue #922). Got it working by adding some directive code to set the zIndex. This leads me to an idea for this issue.

I don't know if this is "bending" the principles too far, but by setting one or even three classes as options to $modal, wouldn't we really be just passing through options to the modalWindow and modalBackdrop directives, which ARE the places to be messing with the DOM and styles, right?

What "far worse" is created by having three class definitions @hall5714? I see many JQuery and Angular Bootstrap modules that have class definitions as part of the passed in options. is it a documentation/comprehension issue? If there is some class interaction issues, we could direct programmers by using names to focus what styles they change, e.g. borderClass, modalSizeClass, etc.

[hallister]

@jmwolfe There are a number of issues here. For starters, we are passing a CSS class name from a controller, to a service, which compiles a directive, which compiles a template, which has the defined class. It's messy. We are doing things via a service that should be done in a view.

And what happens if there is a slight derivation in Bootstrap 3.1 and the modals size is handled in the .modal from that point? Now we have to have two different templates, or we aren't backwards compatible to a minor Bootstrap update because {{ modalSizeClass}} must be on .modal-content in BS 3.0 but in .modal in a figurative 3.1.

In addition a future goal of this project is to support additional frameworks. So using the current Foundation reveal (equivalent of Bootstraps model) and your class names would give us this template:

<div class="reveal-modal {{ borderClass}} {{ modalSizeClass }} {{ modalPositionClass }}"></div>

Three separate classes to modify properties on the same element. Kinda icky. Bottom line is this is a tricky situation. Class definitions do not belong in service options, even if they are being passed into a directive. On the flip side, creating a separate template for each class change requires repetitive coding. I just happen to consider the latter the lessor of two evils. But not everyone will agree with me.

[fxck]

You always need only one class, on the root element. In bootstrap 3 root element happens to be the background layers as well, but who cares.

So it still can be on the .modal element, only in BT3 you are gonna do

.modal .modal-dialog {
   width: yourwidth;
}

and in BT2

.modal {
   width: yourwidth;
}

[hallister]

@fxck True. In fact my first thought was that a simple directive could solve all of this problems:

Which would then transclude the modal template in the span:

<span class="modal-stuff"><div class="modal"></div></span>

So you can do hierarchical selectors to get what you need to get done. That said, as of now $modal attaches to body so that won't work. The up side is it has minimal repetition while simultaneously keeping styling stuff out of the controller/service. But that's a lot of work to avoid class names in a service so 😖

[jmwolfe]

@hall5714 Thanks for taking the time to expand on your concerns. I'm tending towards the repeating templates given all that we need to support. Looking forward to hearing how this plays out.

Seems maybe just providing adequate documentation on how to provide effective templates to support your current framework level could eliminate some problems.

All I know for sure is, I have to have different size dialogs in my app somehow, and soon. :)

[pkozlowski-opensource]

@fxck @hall5714 @jmwolfe thnx for the great discussion here. I gave is some more thought and here is my proposal: let's add a new option, something like windowType or just window. What this option would do is to act as an suffix to the template used by modal window. So, if you don't pass anything we would still load window.html but if people would pass windowType: 'wide', for example, we would include window-wide.html.

This would allow us to keep CSS out of JS and - at the same time - give people total control over their markup. On top of this one wouldn't be obliged to repeat urls of window templates.

In short, the idea is to add an option that would drive a window template to be used.

WDYT?

[fxck]

I'd still rather have css class in js than this. You are gonna get people confused. There will be issues asking why their own template doesn't work, because they forgot to add ng-transclude, they will be asking why it's not animating because they didn't include that ng-class expression.

That's my two cents. But as @jmwolfe said, "All I know for sure is, I have to have different size dialogs in my app somehow, and soon.". So it will do I guess.

[hallister]

@pkozlowski-opensource I like it. Not only does it allow for a custom template (and thus a custom class) but it allows for complete customization of the window without having to define a specific template each time.

@fxck I can see what you are saying with people getting confused, but any implementation we use will cause confusion. And a missing ng-class or a missing ng-transclude is an easy fix when people do have issues.

[pkozlowski-opensource]

@fxck I hear what you are saying... and I guess you are right... at the same time I can also envision people coming and asking for other options to be able to change different parts of the window. So I guess supporting people is inevitable anyway :-)

I'm going to try to implement the solution with the window type described here, release it asap (by this I mean in 2-3 days) and see what is the feedback from the community. We can always change things as this is still not 1.0 release.

[richardcrichardc]

Sounds like a good plan to me.

On 2/09/2013, at 12:43 AM, Pawel Kozlowski notifications@github.com wrote:

@fxck @hall5714 @jmwolfe thnx for the great discussion here. I gave is some more thought and here is my proposal: let's add a new option, something like windowType or just window. What this option would do is to act as an suffix to the template used by modal window. So, if you don't pass anything we would still load window.html but if people would pass windowType: 'wide', for example, we would include window-wide.html.

This would allow us to keep CSS out of JS and - at the same time - give people total control over their markup. On top of this one wouldn't be obliged to repeat urls of window templates.

In short, the idea is to add an option that would drive a window template to be used.

WDYT?

—
Reply to this email directly or view it on GitHub.

[jmwolfe]

Well, best so far!. :) If we are going to give them access to the template, though, why not just expose template and/or templateUrl in the options and let them override the default path. If they don't provide any, it will use the window.html template. What I don't like about it is that it is making the user aware of the internals and also mixing up custom files with the delivered templates. Eh. Not a biggie I guess. I'm just not seeing the point of restricting the location of the supplied template (since they are editing it anyway). Seems to be making more work for them than needed. Now they have to be sure of the format of their filename, etc. etc.

[fxck]

@pkozlowski-opensource the thing is, you can change different parts of the window with that single class. I mean I honestly can't think of any case where I'd need to change anything in the root element. It's pretty much defined by bootstrap what needs a must be there, everything else could should be done from inside.

@jmwolfe's got another point.. I can imaging quite a few possible issues with this solution and only about one with the modalClass and that is natural aversion of putting css in js.

[richardcrichardc]

Here is an idea, how about two extra options:
windowClass - Extra css class to add to window template div.
windowTemplateUrl - Url to load window template from. Defaults to template/modal/window.html which is current hard coded template.

Different people have different requirements. People who want a bit of extra customisation can use windowClass, people who need more have to specify the whole template. People who want to provide the windowTemplate HTML inline can do so by calling $templateCache.put().

WindowClass was 1 1/2 extra lines of code, windowTemplateUrl is probably not much more.

On 2/09/2013, at 6:51 PM, Aleš notifications@github.com wrote:

@pkozlowski-opensource the thing is, you can change different parts of the window with that single class. I mean I honestly can't think of any case where I'd need to change anything in the root element. It's pretty much defined by bootstrap what needs a must be there, everything else could should be done from inside.

@jmwolfe's got another point.. I can imaging quite a few possible issues with this solution and only about one with the modalClass and that is natural aversion of putting css in js.

—
Reply to this email directly or view it on GitHub.

[fxck]

That sounds reasonable. Wouldn't call it "windowClass" though, to me, that sounds like some class you are setting to the body or html tag. I like modalClass or customClass better.

[jmwolfe]

👍

[hallister]

@richardcrichardc @fxck @jmwolfe The reason that having both a windowClass and a windowTemplateUrl is because we are adding two new options that do similar things. windowTemplateUrl makes windowClass irrelevant, since it exposes the window to having class modifications. Any time you have two options that can easily be rolled into one, you do it. A simple API is almost always better.

We don't need to worry about different paths for the window template using @pkozlowski-opensource's method because $templateCache.put() makes the path irrelevant. Yes windowTemplateUrl requires more markup, but we are talking about (at most) three lines of code.

And if you guys are worried about 4 or 5 different templates for a simple width change, you can make a very simple directive that has the class name as a configurable option:

 ...
 scope.className = attrs.className || "";

 $modal({
     ...
     scope: scope,
     windowType: 'custom-width',
     ...
 });
 ...

And your template for window-custom-width.html:

<div class="modal {{ className }} "></div>

Now you have the default modal service and a custom modal directive that handles your class name use case without having a bunch of code. More importantly, it's up to you to define custom styles however you like them, rather than defining them in the API of the service.

[richardcrichardc]

@hall5714. The two options do similar things, but they are for different purposes:

1. windowClass is for people who have tweaked the css in the debugger to get the look they want and want a quick way to apply their extra bit of css
2. windowTemplateClass are for people who want to spend the time understanding how the backdrop, window and content templates interact and maybe make a more substantial change.

You should put that example into the documentation, but hang on, your example HTML is different from the default template:

<div class="modal fade" ng-class="{in: animate}" ng-transclude></div>

The original template has ng-transclude but the example does not? What does ng-transclude do anyway? Is it really safe to leave it out? (Rhetorical questions - no need to reply)

Allowing users to avoid complexity for common use cases is good. It's no help for me, I ended up forking the codebase so I could have different width windows. Maybe we can save some brain-cycles of people who are yet to discover UI Bootstrap.

[hallister]

@richardcrichardc I see what you're saying. The problem is windowType and windowClass perform both actions. So in reality we'd have more than one way to do the exact same thing. That is never a good thing in an API.

I understand that new people coming to Angular may be confused by the template language. That said, we are targeting developers. And with good documentation and examples, the majority are going to be fine. Those that aren't, can open an issue here and will get their answer relatively quickly.

Regardless we are getting fairly rhetorical with what users may or may not run into at some point in the future. Let's let @pkozlowski-opensource run with his plan and we'll revisit this if it becomes an issue.

[richardcrichardc]

@hall5714 I apologise for my flippantness.

Rhetorical is perhaps not what I should have said. These are genuine questions that went through my mind when looking at your example. But I'm not asking you to answer them. I am trying to illustrate that your solution to customising the width of the dialog is not as simple as you make out.

Let's agree we want a simple interface, though disagree on the details.

[hallister]

@richardcrichardc My fault. My example was really lazy to get a quick example out of the way. In reality it would indeed need to be <div class="modal fade {{ className }}" ng-class="{in: animate}" ng-transclude></div>. Not particularly difficult, but not incredibly simple either. That said, I think the copy/paste aspect of this can make it pretty simple overall.

But there's no such thing as the right implementation, just 10 or 12 opinions on which is right 😄

[dmackerman]

So, what's the current best practice way for defining a new width/height on a modal? All of this conversation is great, but this is inflexible at this point and is just a step back from where we were.

EDIT: used @richardcrichardc patch and it's working great. Thanks for putting that together.

[hallister]

@dmackerman It's a WIP right now and it appears it will be done before the next release (soon). Please keep in mind you are working with the master branch of a repository, not an actual release of the product so expect things to workish, the polish will be there prior to release.

[fxck]

While we are at it, different situations might require different overlay opacity, if there was a class option, it could apply the class to the overlay as well, that would fix it. If we were to stick with template option.. well, that would a little bigger pain in the arse.

[tschofen]

Aren't the two choices about two separate scenarios?
A class name let's me tweak the visual presentation. It's simple and straight forward and probably addresses the majority of use cases. And it doesn't really mess with bootstrap (probably the reason people chose it).
The template is for full customization. It takes more understanding and work. As long as the docs make it clear those folks that need it will figure it out.

I'd say we should have both, but for now, being able to tweak things via a class would solve my problems.

[richardcrichardc]

@fxck By the overlay I assume you mean the backdrop. I don't think the patch I provided for the class will help with that, as there is a separate template for the backdrop. The 'tweak' class could instead be applied to the backdrop, then a selector like .tweak-class modal could be used to tweak the modal - actually I don't think that would work as I don't think the backdrop wraps the modal.

You can see what I mean by examining the code:

--- modal.js
+++ modal.js
@@ -138,7 +138,7 @@
           backdropDomEl = $compile(angular.element('<div modal-backdrop></div>'))($rootScope);
           body.append(backdropDomEl);
         }
-        var modalDomEl = $compile(angular.element('<div modal-window></div>').html(modal.content))(modal.scope);
+        var modalDomEl = $compile(angular.element('<div modal-window></div>').html(modal.content).addClass(modal.windowClass))(modal.scope);
         body.append(modalDomEl);

Oh oh. Two 'tweak' options ;-). @FYCK Is different overlay opacity between modals a likely requirement. I don't think I would ever want to do that. I would want to tweak the overlay opacity globally, but that doesn't need an extra class. The only use case I can think of is to distinguish dialogs, .e.g. bright red danger overlay for dialogs the user really should pay attention to, but that effect could be produced just as well inside the dialog.

I still think the most pragmatic approach is (or similar):

1. windowClass - Extra css class to add to window template div.
2. windowTemplateUrl - Url to load window template from. Defaults to template/modal/window.html which is current hard coded template.

[fxck]

@richardcrichardc yes I meant backdrop and yes it does not wrap modal(well in bootstrap 3 it actually does).

[pkozlowski-opensource]

Guys, I've just pushed a change that introduces windowClass option, as it seems to be a preferred solution by many people lurking into this thread. I don't want to hold a release any longer so let's get 0.6.0 out of the doors and see if there are real use-cases that we can't cover with the existing solution.

If anyone could give this thing a try it would be awesome.

[tschofen]

Just tested it…and fixes my UC nicely (modal window sizing).

[chennoam7]

Hi,

Can anyone show me how to use the new feature to setup a different modal width than the default?
I tried to change the width, using windowClass, with no luck as follows:
$stateProvider.state("signin", {
url: "/signin",
onEnter: function ($stateParams, $state, $modal) {
$modal.open({
templateUrl: 'Pages/Modals/Signin.html',
windowClass: 'login-modal',
//resolve: {
// item: function () { new Item(123).get(); }
//},
controller: ['$scope', function ($scope) {
$scope.dismiss = function () {
$scope.$dismiss();
};

                    //$scope.save = function () {
                    //    item.update().then(function () {
                    //        $scope.$close(true);
                    //    });
                    //};,
                }]
            }).result.then(function (result) {
                if (result) {
                    return $state.transitionTo("/");
                }
            });
        }
    });

My class is setup as follows:
login-modal {
width:270px;
}

Thank you,
Chen

[jeffjohnson9046]

@chennoam7: I just went through a similar exercise for changing the width of a modal window. You might try changing your CSS to this:

login-modal .modal-dialog {
width: 270px;
}

[brianfeister]

@chennoam7 and anyone else who stumbles upon this seemingly unresolved thread - the resolution was the addition of windowClass option on the $modal.open() method.

Implementation as below worked for me and gave a class of full-screen to <div class="modal full-screen ...">

      var modalInstance = $modal.open({
        templateUrl: 'myModalTemplate.html',
        controller: this.ModalInstanceCtrl,
        windowClass: 'full-screen',
        resolve: {
          items: function () {
            return $scope.items;
          }
        }
      });

[vanslly]

Hey Guys, I'm really glad you added some extensibility here with the windowClass option, however this change does not play nice with the bootstrap 3.1.1 way of using the modal-lg CSS class on the inner div of the window element:

<div class="modal">
    <div class="modal-dialog modal-lg">
        <div class="modal-content">

        </div>
    </div>
</div>

Any chance of introducing one more option?

@brianfeister Thanks for spelling it out for me, it's a long thread.

[gazoakley]

I've just come across this after trying to do the same thing as @vanslly - and whilst @jeffjohnson9046's solution works it'd be easier to just allow using the classes bundled with Bootstrap instead of adding another snippet of CSS as a workaround. I'm happy to fork and submit a pull request if people think this is a good solution.

[ghost]

.modal { width: auto; min-width: yourwidth; display: inline-block;}
Set Inside new "div" with whose width will be stretched

[shashikantj]

@vanslly there is a size option for modal

$modal.open({
  controller : 'myModalController',
  size : 'lg',
  ...
})