Opened 11 years ago

Closed 11 years ago

#7853 closed enhancement (fixed)

[patch] [cla] dojox.image.LightboxNano

Reported by: cb1kenobi Owned by: dante
Priority: high Milestone: 1.3
Component: Dojox Version: 1.2.0
Keywords: Cc:
Blocked By: Blocking:

Description

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.

Attachments (1)

hooray4ie6.png (116.7 KB) - added by dante 11 years ago.
a bgIframe should be optionally supported?

Download all attachments as: .zip

Change History (8)

comment:2 Changed 11 years ago by Adam Peller

Owner: changed from Adam Peller to dante

comment:3 Changed 11 years ago by 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

comment:4 Changed 11 years ago by 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.

Changed 11 years ago by dante

Attachment: hooray4ie6.png added

a bgIframe should be optionally supported?

comment:5 Changed 11 years ago by 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?.

comment:6 Changed 11 years ago by 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.

comment:7 Changed 11 years ago by dante

Milestone: tbd1.3
Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.