Dialog Provider #36

typesafe · 2012-12-25T10:48:31Z

This PR replaces the previous one I closed.

I moved all dialog work to a feature branch, and then rebased and squashed it all into a single commit on master.

I also updated the code to use the new $transition service.

petebacondarwin · 2012-12-26T08:16:01Z

Nice work Gino. How was $transition?

... sent from my tablet
On 25 Dec 2012 10:48, "Gino Heyman" [email protected] wrote:

This PR replaces the previous onehttps://github.com/angular-ui/bootstrap/pull/25I closed.

I moved all dialog work to a feature branchhttps://github.com/typesafe/bootstrap/commits/dialog,
and then rebased and squashed it all into a single commithttps://github.com/typesafe/bootstrap/commits/masteron master.

I also updated the code to use the new $transition service.

You can merge this Pull Request by running:

git pull https://github.com/typesafe/bootstrap master

Or view, comment on, or merge it at:

#36
Commit Summary

feat(dialog): merged feature branch into master

File Changes

A docs/dialog.html (231)

A docs/docco.css (192)

A src/bootstrap.js (28)

A src/dialog/README.md (92)

A src/dialog/demo/app.js (51)

A src/dialog/demo/index.html (71)

A src/dialog/dialog.js (267)

A src/dialog/test/dialog.js (285)

A template/dialog/message.html (9)

Patch Links

https://github.com/angular-ui/bootstrap/pull/36.patch

https://github.com/angular-ui/bootstrap/pull/36.diff

—
Reply to this email directly or view it on GitHubhttps://github.com/Dialog Provider #36.

typesafe · 2012-12-26T08:20:08Z

Worked like a charm! :-)

petebacondarwin · 2012-12-26T10:14:00Z

You shouldn't need src/bootstrap.js now then?

typesafe · 2012-12-26T10:30:58Z

Exactly, forgot all about that one. src/bootstrap.js should be deleted.

pkozlowski-opensource · 2012-12-29T21:40:44Z

@typesafe Sorry for the delay but finally I had some time to have a closer look into your PR (which I want to merge ASAP as it contains great stuff!!!).

So, pre-merged this PR into a separate branch (pr36) and there are things that need more love:

some tests are failing on Safari, full report here: http://ci.angularjs.org/job/angularui-bootstrap/43/console
we need to get rid of the src/bootstrap.js file
the demo part needs changes, it should contain partials only so the final site can be generated from those partials. Please have a look at the accordion folder to see what I mean (and don't hesitate to ping me if needed).

Once again, thnx for all the hard work, this is great stuff, we are close to landing it.

typesafe · 2013-01-01T18:51:05Z

@pkozlowski-opensource, I'll look into this as soon as I find some time in the coming days!

pkozlowski-opensource · 2013-01-01T19:28:49Z

@typesafe cool, don't stress! We are working toward the very first release and I would love to land your PR but once again, it is better to wait few more days and not to rush things!

typesafe · 2013-01-02T12:37:59Z

@pkozlowski-opensource, I just ran all tests on Safari (version 6.0, on my MBP) and they all passed...

Safari 6.0: Executed 96 of 96 SUCCESS (0.759 secs / 0.667 secs)
Chrome 23.0: Executed 96 of 96 SUCCESS (1.021 secs / 0.827 secs)
TOTAL: 192 SUCCESS

I ran them from both my local repo, as well as from a downloaded copy of the pr36 branch. Are you aware of any particularities with the Jenkins CI build?

pkozlowski-opensource · 2013-01-02T18:52:22Z

@typesafe bah, don't know what is going on with those Safari tests (although I saw some strange things going on already).

Could you do other changes (remove the src/bootstrap.js and change the demo so it is a partial) so we can try once again. I'm installing Safari to test on my end.

pkozlowski-opensource · 2013-01-02T19:59:24Z

@typesafe just run the tests on my local machine and things are OK (Windows, Safari 5.1). Really not sure what is going on here...

Could you do other changes so we can have a look at what is going on?

removed docco /docs files and /src/bootstrap.js and added /src/dialog/docs files for the demo page.

typesafe · 2013-01-03T14:12:48Z

@pkozlowski-opensource, here it is. Feel free to edit the demo page.

Let's hope Safari behaves better this time...

pkozlowski-opensource · 2013-01-04T10:32:30Z

@typesafe Gino, grr, seems like there are still problems with the CI build for Safari:
http://ci.angularjs.org/job/angularui-bootstrap/72/console

I'm not sure what is going on as those tests pass on my side :-(

I will try to look into this later today / contact guys at Google but if you've got any ideas feel free to submit patches to the PR.

pkozlowski-opensource · 2013-01-04T10:32:56Z

BTW: tested demo locally and it is simply AWESOME!!!!

pkozlowski-opensource · 2013-01-04T10:58:53Z

@typesafe I think I know what is going on, it must be linked to some clean-up not happening to which Safari is vulnerable. I've managed to reproduce it locally by capturing multiple browsers (in other words, this test starts to fail as soon as you capture multiple browsers).

You can see this by doing:

grunt server
capture multiple browsers at http://localhost:9018/ (I've captured Chrome, FFox and Safari as the last one)
run tests with grunt test-run

So, at the end of the day there is a real issue here, CI server is working just fine.
BTW: we've got this issue opened: #13 maybe it is linked?

typesafe · 2013-01-04T16:57:23Z

@pkozlowski-opensource, I used pretty much every browser I could find on my local network and combined it in several ways...

Here's a sample of the (inconsistent) results while (somewhat) simulating the CI env (run testacular, run tests once, then stop the whole thing):

Chrome 23.0: Executed 96 of 96 SUCCESS (0.973 secs / 0.781 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.7 secs / 0.595 secs)
TOTAL: 192 SUCCESS

...

Chrome 23.0: Executed 96 of 96 SUCCESS (2.083 secs / 1.673 secs)
Chrome 23.0: Executed 96 of 96 SUCCESS (3.652 secs / 1.437 secs)
IE 10.0: Executed 96 of 96 SUCCESS (3.494 secs / 1.524 secs)
Firefox 15.0: Executed 96 of 96 SUCCESS (2.388 secs / 1.979 secs)
Safari 5.0: Executed 96 of 96 SUCCESS (7.34 secs / 6.076 secs)
Safari 6.0: Executed 96 of 96 (13 FAILED) (1.664 secs / 1.198 secs)
TOTAL: 13 FAILED, 563 SUCCESS

...

Firefox 16.0: Executed 96 of 96 SUCCESS (1.34 secs / 1.165 secs)
Safari 6.0: Executed 96 of 96 (13 FAILED) (0.874 secs / 0.715 secs)
Chrome 23.0: Executed 96 of 96 SUCCESS (1.168 secs / 0.928 secs)
TOTAL: 13 FAILED, 275 SUCCESS

...

Firefox 16.0: Executed 96 of 96 SUCCESS (1.014 secs / 0.903 secs)
Safari 6.0: Executed 13 of 96 (skipped 83) SUCCESS (0.148 secs / 0.076 secs)
TOTAL: 109 SUCCESS

...

Firefox 10.0: Executed 96 of 96 SUCCESS (1.41 secs / 1.004 secs)
Chrome 23.0: Executed 96 of 96 SUCCESS (1 min 0.249 secs / 46.834 secs)
Firefox 16.0: Executed 96 of 96 SUCCESS (1.323 secs / 1.202 secs)
Safari 6.0: Executed 96 of 96 (13 FAILED) (0.829 secs / 0.746 secs)
IE 9.0: Executed 95 of 95 SUCCESS (1.255 secs / 0.825 secs)
TOTAL: 13 FAILED, 466 SUCCESS

...

Chrome 23.0: Executed 96 of 96 SUCCESS (0.953 secs / 0.606 secs)
Firefox 17.0: Executed 96 of 96 SUCCESS (1.343 secs / 1.152 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.843 secs / 0.678 secs)
TOTAL: 288 SUCCESS

On some runs, Chrome took a LONG time to complete the tests, but they were successful nonetheless... Note that it is only Safari 6 that fails the tests (and not only as last one in the queue).

Then I started leaving testacular alone (run grunt server and grunt watch, and kept them running) and just save a file without any further changes to let the watching kick in, and guess what? More inconsistencies...

Save dialog.js (we were lucky :-)):

Chrome 23.0: Executed 96 of 96 SUCCESS (0.938 secs / 0.573 secs)
Firefox 17.0: Executed 96 of 96 SUCCESS (1.088 secs / 0.903 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.668 secs / 0.571 secs)
TOTAL: 288 SUCCESS

Save file again (not so lucky, however - no changes):

Chrome 23.0: Executed 96 of 96 (13 FAILED) (0.869 secs / 0.557 secs)
Firefox 17.0: Executed 96 of 96 SUCCESS (1.108 secs / 0.929 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.643 secs / 0.572 secs)
TOTAL: 13 FAILED, 275 SUCCESS

Save file again:

Chrome 23.0: Executed 13 of 96 (skipped 83) SUCCESS (0.439 secs / 0.072 secs)
Firefox 17.0: Executed 96 of 96 SUCCESS (1.204 secs / 1.015 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.808 secs / 0.681 secs)
TOTAL: 205 SUCCESS

And again... (WTF?!)

Chrome 23.0: Executed 96 of 96 (13 FAILED) (0.954 secs / 0.605 secs)
Firefox 17.0: Executed 96 of 96 SUCCESS (1.104 secs / 0.948 secs)
Safari 6.0: Executed 96 of 96 (13 FAILED) (0.682 secs / 0.611 secs)
TOTAL: 26 FAILED, 262 SUCCESS

I'm starting to think this is a testacular issue... but then again, why is it mostly Safari and sometimes another webkit browser? Weird...

Then I altered the grunt file to use testacular's --auto-watch flag. This resulted in the first run (the first run after issuing grunt server) to fail EVERY TIME (note that this time it is FF):

Firefox 17.0: Executed 96 of 96 (13 FAILED) (1.034 secs / 0.87 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.615 secs / 0.56 secs)
TOTAL: 13 FAILED, 179 SUCCESS

However, ALL subsequent runs (invoked by saving an unchanged files) were successful (no matter what browser combo, this is just a sample):

info (watcher): Changed file "/Users/gheyman/bootstrap/src/dialog/dialog.js".
Firefox 17.0: Executed 96 of 96 SUCCESS (1.22 secs / 1.046 secs)
Safari 6.0: Executed 96 of 96 SUCCESS (0.739 secs / 0.622 secs)
Chrome 23.0: Executed 96 of 96 SUCCESS (1.031 secs / 0.943 secs)
TOTAL: 288 SUCCESS

I'll try to get to the bottom of this. But any help is much appreciated! ;-)

pkozlowski-opensource · 2013-01-04T17:09:15Z

@typesafe

2 quick remarks:

I saw slow executions in Chrome what DOM is attached to a document. This makes testacular "stuck" for a longer while. Usually switching (visually) to a tab running tests makes it unstuck. I need to ask Vojta on the mailing list about it.
Testacular won't execute all the tests on subsequent runs if a tests fail. Which means that on subsequent runs less tests are executed which means that there are less things to interfere. This is not very reliable then.

I will try to look at this during the weekend. I'm still suspecting some cleanup issue.

pkozlowski-opensource · 2013-01-04T17:42:11Z

@typesafe just one thought - didn't try this - but maybe it is backdrop that stays attached to the body and not removed?

typesafe · 2013-01-05T13:12:49Z

It is indeed the backgrop that remains attached to the DOM... with display:none. Been debugging in Chrome and Safari, but no luck yet. It's like there's a race condition (which seems impossible) somewhere, dunno, it's really strange...

On the bright side, it is easily and consistently reproducible with the exception of mostly it's Safari sometimes it's Chrome and very rarely it's FF.

One way to avoid the issue, would be to only create the backdrop element when it's needed. That would require a bit of 'bookkeeping code', but that's it. I'll try to get to the bottom of this later today (if I can free the time), but I'm afraid it'll remain a mystery :-(. Anyway, there's a solution in the make!

pkozlowski-opensource · 2013-01-05T16:05:33Z

@typesafe ok, good that we know what is going on. I was also thinking that it might be other modal directive interfering here.

I think that adding and removing the backdrop element as needed is acceptable. Alternatively we could remove this element after each test (afterEach), no? Didn't try this, just a thought...

typesafe · 2013-01-06T10:36:32Z

@pkozlowski-opensource, there already is proper clean-up in the dialog tests:

// clean-up after ourselves
afterEach(function(){
    closeDialog();
    clearGlobalOptions();
});

But you were right on the modal directive interfering! For some reason at first run testacular does not clean-up the context iframe properly (something to look at I guess) and the backdrops introduced by the modal directive tests remained present in the DOM!

Adding this to the modal directive tests seems to resolve the issue:

afterEach(function(){
    $('.modal-backdrop').remove();
});

That was a nasty one! I'll try to push an update today...

pkozlowski-opensource · 2013-01-06T10:51:40Z

@typesafe glad that you've managed to pin-point it! It was a tough one, I must admit!

But at least now you know everything about Jasmine tests and testacular :-)

Added an afterEach to the modal directive tests that removes all modal-backdrop divs from the DOM. They sometimes interfere with the dialog tests (making 13 of them fail), probably due to a testacular glitch.

alexhanh · 2013-01-13T23:19:28Z

Tried putting the demo up on JSFiddle but it's not working for me on Chrome. http://jsfiddle.net/9gYGf/

pkozlowski-opensource · 2013-01-14T10:35:50Z

@alexhanh Thnx for looking into this one but the issue was thet your jsFiddle wasn't well configured, there were multiple problems really, but the most important ones were:

there was no ng-app tag
there were no module declared with a dependency on the 'ui.bootstrap.dialog' module
there was not template for the message box

Here is a fixed jsFiddle: http://jsfiddle.net/9gYGf/2/

Hopefully I will have some time to merge this PR later today so we will have a demo, docs etc.

pkozlowski-opensource · 2013-01-14T11:48:52Z

@typesafe Finally managed to merge this PR, all the tests are passing now! It landed as 9c06f62 (after squashing commits and changing commit message).

Thank you sooo much for this pull request! Now we can start working on popovers following the same approach!

typesafe · 2013-01-14T12:25:04Z

Thanks @pkozlowski-opensource. Time to update my apps :-).

BrainCrumbz · 2013-03-08T18:38:40Z

I thought, reading docs and examples, that messageBox didn't need a template, Infact you don't see any template mentioned for it in https://github.com/angular-ui/bootstrap/blob/master/src/dialog/README.md or in http://angular-ui.github.com/bootstrap/#/dialog.
Also, looking at source code in ui-bootstrap-tpls-0.2.0.js, it seems that the template is somehow defined in a "templateCache".

But then, if I don't define any template, the default one is not picked up and I see a 404 error as browser tries to load template/dialog/message.html from localhost. In order to solve that, I manually copied in my HTML page the template taken from the source code:

<script id="template/dialog/message.html" type="text/ng-template">
    <div class="modal-header">
        <h1>{{ title }}</h1>
    </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>
</script>

Maybe in next versions you want to make that more clear.

Thanks for all this great job you're doing!

feat(dialog): merged feature branch into master

d563f21

feat(dialog) removed obsolete files and added demo page files

9f27f79

removed docco /docs files and /src/bootstrap.js and added /src/dialog/docs files for the demo page.

pkozlowski-opensource mentioned this pull request Jan 3, 2013

add fade-in and fade-out support for modal #50

Closed

Fixed testacular issue

eb0a240

Added an afterEach to the modal directive tests that removes all modal-backdrop divs from the DOM. They sometimes interfere with the dialog tests (making 13 of them fail), probably due to a testacular glitch.

pkozlowski-opensource closed this Jan 14, 2013

Neilski mentioned this pull request Oct 14, 2015

uib-datepicker-popup formatting problem #4616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dialog Provider #36

Dialog Provider #36

typesafe commented Dec 25, 2012

petebacondarwin commented Dec 26, 2012

I also updated the code to use the new $transition service.

typesafe commented Dec 26, 2012

petebacondarwin commented Dec 26, 2012

typesafe commented Dec 26, 2012

pkozlowski-opensource commented Dec 29, 2012

typesafe commented Jan 1, 2013

pkozlowski-opensource commented Jan 1, 2013

typesafe commented Jan 2, 2013

pkozlowski-opensource commented Jan 2, 2013

pkozlowski-opensource commented Jan 2, 2013

typesafe commented Jan 3, 2013

pkozlowski-opensource commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

typesafe commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

typesafe commented Jan 5, 2013

pkozlowski-opensource commented Jan 5, 2013

typesafe commented Jan 6, 2013

pkozlowski-opensource commented Jan 6, 2013

alexhanh commented Jan 13, 2013

pkozlowski-opensource commented Jan 14, 2013

pkozlowski-opensource commented Jan 14, 2013

typesafe commented Jan 14, 2013

BrainCrumbz commented Mar 8, 2013

Dialog Provider #36

Dialog Provider #36

Conversation

typesafe commented Dec 25, 2012

petebacondarwin commented Dec 26, 2012

I also updated the code to use the new $transition service.

typesafe commented Dec 26, 2012

petebacondarwin commented Dec 26, 2012

typesafe commented Dec 26, 2012

pkozlowski-opensource commented Dec 29, 2012

typesafe commented Jan 1, 2013

pkozlowski-opensource commented Jan 1, 2013

typesafe commented Jan 2, 2013

pkozlowski-opensource commented Jan 2, 2013

pkozlowski-opensource commented Jan 2, 2013

typesafe commented Jan 3, 2013

pkozlowski-opensource commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

typesafe commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

pkozlowski-opensource commented Jan 4, 2013

typesafe commented Jan 5, 2013

pkozlowski-opensource commented Jan 5, 2013

typesafe commented Jan 6, 2013

pkozlowski-opensource commented Jan 6, 2013

alexhanh commented Jan 13, 2013

pkozlowski-opensource commented Jan 14, 2013

pkozlowski-opensource commented Jan 14, 2013

typesafe commented Jan 14, 2013

BrainCrumbz commented Mar 8, 2013