Popup

Example

Class

Tests

Note that this ticket does not yet have the necessary CSS and graphics attached.

sbenthall, thanks for this contribution. Here are my comments:

• Why bind the popup to a feature? This is ok as an option, but why not also allow just a LonLat? It would be no extra effort at all, except for an if check.
• Why force the popup to live inside a MapPanel? The big advantage compared to the native OL popup is that it could easily live outside the MapPanel (i.e. taking space outside the map viewport). I don't think it would be too much effort to change the position() method to allow for putting the popup on parent containers of the MapPanel. While doing so, the panIntoView method could be cleaned up. I don't think the padding properties from OpenLayers are needed, IIRC Ext knows very well how large it's windows are.
• The anc and ancSrc properties are not advertised, and several ND comments are missing or incorrect.

Further comments on the example:

• we use different paths in the MapPanel example, but we have not decided yet upon a recommended structure (especially where we expect to find ext and openlayers). This would be good to discuss on the dev list
• instead of items.items[0], you should use items.get(0).

Comments on the unit tests:

• commented out test cases from other tests should be removed
• you should add tests for destroy, since you say in the beforeDestroy method of the class that you are not sure if this is the right way to do it.

Apart from that: thanks for the good work, and next time please provide a complete patch with correct paths in the example, plus all css files and images. There is a thread on the dev list to discuss how we want to handle css and image paths (http://www.geoext.org/pipermail/dev/2009-March/000010.html).

Sorry Seb, I see now I missed the point of the padding in panIntoView. I think the padding should be a config option in the popup class.

Another note: will there be more stuff in the GeoExt.Popup namespace? If not, we don't need an extra namespace.

Thanks for your review, Andreas.

Some questions:

• The feature request about letting popups appear outside the MapPanel has already been ticketed as #8. While I agree that this is a good feature, is there a reason why inclusion of Popups in the library should block on this feature? I know that personally I won't have a lot of time in the near future to apply towards this code, so I would would rather not be hung up on "It would be nice if..." requests.
• Could you tell me which ND comments are incorrect, or point me to how I can better check for myself? Where is the spec for ND?
• There is a chicken and egg problem w/r/t my writing tests for destroy(). What I'm not sure about is whether what I'm trying to do with beforeDestroy() is correct, not whether the code does what I'm trying to do. Does the content of beforeDestroy() look right to you? If so, I'll happily write some tests.

I agree that it would be good to standardize the paths for examples, as well as for tests (last I checked, the tests for MapPanel couldn't run in FF2 without modification).

Replying to sbenthall:

* The feature request about letting popups appear outside the MapPanel has already been ticketed as #8. While I agree that this is a good feature, is there a reason why inclusion of Popups in the library should block on this feature? I know that personally I won't have a lot of time in the near future to apply towards this code, so I would would rather not be hung up on "It would be nice if..." requests.

You are right, I missed that other ticket. I only hope that resolving #8 does not require substantial changes to the Popup class, but is a mere addition.

* Could you tell me which ND comments are incorrect, or point me to how I can better check for myself? Where is the spec for ND?

ND itself is simple (see http://naturaldocs.org). The things that are wrong are more in the way you are (not) using ND, so please check your code for the following:

• Does every property, constant, method and function have a comment starting with /** (not /*)? Marking methods as // private is what ExtJS does, but we still use ND for these methods. The difference is that we don't mark them as APIMethod, but just as Method. This does not mean that all other methods are APIMethods, because we usually start with Methods and change them to APIMethods once we are confident that they won't change anymore and people should use these.
• Does every ND comment explain the arguments and the return value (if any)?

* There is a chicken and egg problem w/r/t my writing tests for destroy(). What I'm not sure about is whether what I'm trying to do with beforeDestroy() is correct, not whether the code does what I'm trying to do. Does the content of beforeDestroy() look right to you? If so, I'll happily write some tests.

To me it feels that most of the beforeDestroy content should go in a separate method removeFromMap(Panel), which would be called from beforeDestroy. Then it would do the opposite of what addToMap(Panel) does. I hope this answers your question.

I agree that it would be good to standardize the paths for examples, as well as for tests (last I checked, the tests for MapPanel couldn't run in FF2 without modification).

Thanks for starting a thread about this on the dev list. I hope we will have a solution soon.

full patch

New patch attached. Notes:

• David Winslow added positioning by LonLat as configuration awesome.
• Paths and structure should be up to date with latest directory structure.
• Unfortunately, I've not had time to work on the feature described in #8, nor have I been able to make map padding a configuration option yet.
• There are now tests for the beforeDestroy method, which calls an unbindFromMapPanel method which performs its former functionality (and is used elsewhere)
• I believe ND are complete
• The example is in the Ext style, with the javascript loaded from a separate file.

(In [287]) Popups for the GeoExt MapPanel. Thanks sbenthall for the hard work on this! The changeset has the following differences from the original patch (popup.patch):

• calls super in beforeDestroy()
• simplified this.on() in anchor()
• added/modified some ND comments, especially a Constructor keyword
• simplified map creation in the example
• added link to the js file in the example
• removed unneeded newlines

r=me (closes #21)

r287 also adds the anchor images for the default and gray themes, taken from the drake sandbox.

Popup trouble on IE (6 at least). Example doesn't work and tests fail.

I think some of the IE errors are due to the issue described in #39. (Others are not--I'm working on a patch now).

Attached patch popup.ie.patch makes the following changes:

• checks to see if the second argument, h, isNaN before doing math on it. Ext's setSize method apparently works if h is undefined but not if it's a NaN
• png08 anchor images for the popups, thanks to rpenate. Should be translucent when possible and otherwise degrade gracefully.
• some semi-colons and other responses to jslint

I think that the test errors are due to the issue described in #39.

updated IE patch

New patch has passing tests in IE (worked around, didn't solve, #39 issue)

(In [479]) IE fixes for Popups. Tests now pass. Example works. Anchor graphic transparency works. Thanks to rpenate for the images. r=tschaub (Closes #21)