alternative fix for 'overlay only works once'

[pirxpilot]

fixes #21
alternative to #30

[dy]

It seems that it works well, but honestly I don’t like an extra state _hideTrigger you introduced - I thought about the same solution.
The problem of that is that it actually covers a symptom, not the real trouble here ). _hideTrigger is rather a hack than a real state, a cover to overlay’s "imaginary" isHidden state. Both 'hide' callback and hideOverlay now have polymorphic behaviour depending on _hideTrigger, which becomes obsolete once overlay will solve it’s API issues. So this is a temporary solution, IMO. Though valid.
I still think that normalizing overlay’s API is as well important. At least to prevent errors on multiple hide calls.

[dy]

@pirxpilot we have to make up a decision on that)

[dy]

I don’t mind merging any of PR, providing that overlay is also fixed. Decide to yourself which solution looks simpler and clearer )

[pirxpilot]

Yup - _hideTrigger was kind of lame. How about this one.
Check it out: make sure you run test/reuse.html - and close the dialog using all methods (Esc/Close/Overlay click)

[dy]

It works.
There are changed API and a separate test case - it is minor change, not a patch.
I think it’s a good idea to introduce a new method showOverlay to balance hideOverlay, but is there a reason/use-case for that? I can’t think up anything, even hideOverlay seem redundant for me).
Also - how is it profitable to create overlay each time dialog is shown and clear it each time it’s hidden? Why not to keep reference once overlay is created?

IMO that new version looks more obscure than is was initially. Factually some new hidden state is defined via _overlayOptions, what makes code of show method quite unclear.
I just commented reader’s thoughts:

Dialog.prototype.show = function(){
var overlay = this._overlay;

this.showOverlay();
//↑ok, but why showOverlay if I have not defined it for my instance?
//Where is the check of it’s flag?
//Probably within this.showOverlay.
//But at least that is unnecessary call then. 
//Ok, I see `if (!self._overlayOptions) return;` in showOverlay
//so probably overlay is defined via this._overlayOptions.

if (!overlay || overlay.closable) this.escapable(); 
//here the state of overlay is checked via this._overlay
//so what is the overlay flag - this._overlayOptions or this._overlay?

Such confusion makes things very difficult to track and understand.

The only critical thing here seems to be in .once, which is the same as initial suggestion.

No offense, but I like it lesser than your previous hidden state version, it is too verbose )
But again - the solution is valid, and if you insist we can merge it.

[pirxpilot]

There are changed API and a separate test case - it is minor change, not a patch.

Could you elaborate on that. I don't think we changed any public/documented API but maybe I am missing something.

RE: creating/destroying overlay every time: I don't see any other obvious way of supporting dialog reuse - #30 does not really work well when dialog is closed (multiple times) from the close button.

Let's give a day or two for other contributor comments though before squashing this branch and taking PR into master.
After all #21 was open since forever...
Ping: @Swatinem @KenanSulayman @stephenmathieson @timaschew

[Swatinem]

Code looks good, but I haven’t tried or tested this at all.

[dy]

@pirxpilot so are we going to merge it?

[Swatinem]

Alright, just did :-)