[patch] [cla] dojox.image.LightboxNano

[cb1kenobi]

Here's the LightboxNano? that I showed off at DDD 2008 in Boston. I'm thinking dojox.image would be a good home for it next to the current Lightbox.

I've attached both a patch and a tar.gz of the files. Not sure if the binary files work in a patch file.

[cb1kenobi]

Ugh... 256kb upload limit? Really?

You can download them from my site:

Patch: http://www.cb1inc.com/files/LightboxNano.patch
tar.gz: http://www.cb1inc.com/files/LightboxNano.tar.gz

View the demo!

http://www.cb1inc.com/dojo-1.2/dojox/image/tests/test_LightboxNano.html

[msmith]

Nice work Chris. Nice and simple. Looks like a great candidate to be included.

In playing with it a bit, I was able to cause a state where the page was unusable due to the overlay node. In particular, right after loading the page, if you double-click an image, the overlay will load twice and the image will be enlarged. After dismissing the image, only one overlay is dismissed, and one remains, preventing any other interactions in the page and maintaining the "grayed-out" background. I was only able to reproduce this immediately after loading the page.

I'm on Vista/FF3.

Send me a message to msmith on the dojo forums if you need more info.

~Michael

[cb1kenobi]

I've updated the test to include a programmatic example. I also fixed the double-click bug that msmith found (thanks msmith!).

I've update the patch and tar.gz on the CB1 website.

[dante]

a bgIframe should be optionally supported?

[cb1kenobi]

I updated the code with:

* a cute loading icon
* image preloading... it waits 5 seconds (can be overridden) after the LightboxNano? is created before preloading to give time for other page elements to load
* is no longer a dijit._Widget * re-focus the original image after closing for keyboard accessibility

I've updated the patch, tar.gz, and test page on the CB1 website.

As for the iframe, it's probably not a bad idea, but I had adding a hack for a soon to be obsolete browser when the whole concept of the LightboxNano? is to be lightweight. In other words, I think the widget should be accepted into dojox, but implementing a iframe for IE6 should be a low priority.

Mental note: I had to duplicate the dijit.getViewport() to avoid requiring dijit. getViewport() needs to be in the dojo namespace badly. When getViewport() is moved into the dojo ns, we need to update the LightboxNano?.

[dante]

Couple of notes:

So I'd made a style pass at this already, wanting to commit it as it was before the previous comment was made, and now the code has changed, so: My notes from that are:

* The class added to the node should be prefixed with the word 'dojox'. dojoxEnlarge or something similar, as we try to name our classes things no one will use in user code ever, prefixing with a namespace avoids conflicts.

* dojo.style() calls with objects should list each property on a newline. this and other minor cleanups like "whitespace should be used excessively, the build removes them" in between mathematical functions and object key/values. It's all outlined on the style guide on dtk.org

* using _t for a reference to 'this' - obfuscation? minor nit, but in the interest of readability _this will suffice, and be obfuscated as part of the build process. just a suggestion.

* I don't like the idea of duplicating getViewport in a widget just to avoid a dijit dependency. dojo.require("dijit._base.place") would be the immediate quick solution, though includes slightly more code than you'd like. another option would be to take advantage of JS and put your viewport code as a private inside of a closure, taking advantage of not recreating the function for each instance.

* removing the dijit._Widget is a little confusing. Its not bad at all, but you cross a boundary by excluding it. Now the "common methods" and the way we've taught everyone to use and extend widgets (not classes, widgets) is broken. Can you .destroy() a lightboxnano? how do you disable it?

* the bgIframe thing I _bet_ will be requested. I suggested providing a hook that makes the assumption of the availablility of the code (eg: the user has to explicitly require the bgIframe code, and turn on a flag in the widget instance to enable it. It should be fairly simple either way. Your estimations of how quickly ie6 will go away amuse me. ;)

* you should write up user docs to cover the useage and bgIframe notes at:

http://docs.dojocampus.org/dojox/image/LightboxNano

You can't do "live" inline example until it's in trunk (and campus updates their version) but putting the content in place would be a a huge win, and a pattern I'd like to follow for all new functionality, dojox included.

[dante]

thanks cb1 - self-commited and trac didn't close this. fixed in [15602]