Opened 9 years ago

Closed 9 years ago

#21 closed enhancement (fixed)

Popup

Reported by: sbenthall Owned by: tschaub
Priority: major Milestone: 0.5
Component: GeoExt.Popup Version:
Keywords: Cc:
State: Review

Description

A Popup widget. Popups are specialized Windows that can be anchored to and unanchored from a feature on a map.

Attachments (5)

popup.html (3.2 KB) - added by sbenthall 9 years ago.
Example
Popup.js (8.4 KB) - added by sbenthall 9 years ago.
Class
Popup.html (6.5 KB) - added by sbenthall 9 years ago.
Tests
popup.patch (22.9 KB) - added by sbenthall 9 years ago.
full patch
popup.ie.patch (5.3 KB) - added by sbenthall 9 years ago.
updated IE patch

Download all attachments as: .zip

Change History (22)

Changed 9 years ago by sbenthall

Example

Changed 9 years ago by sbenthall

Class

Changed 9 years ago by sbenthall

Tests

comment:1 Changed 9 years ago by sbenthall

  • Type changed from defect to enhancement

comment:2 Changed 9 years ago by sbenthall

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

comment:3 Changed 9 years ago by ahocevar

  • State changed from Review to Needs more work

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).

comment:4 Changed 9 years ago by ahocevar

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.

comment:5 follow-up: Changed 9 years ago by sbenthall

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).

comment:6 in reply to: ↑ 5 Changed 9 years ago by ahocevar

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.

Changed 9 years ago by sbenthall

full patch

comment:7 Changed 9 years ago by sbenthall

  • State changed from Needs more work to Review

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.

comment:8 Changed 9 years ago by ahocevar

  • Resolution set to fixed
  • Status changed from new to closed

(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)

comment:9 Changed 9 years ago by ahocevar

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

comment:10 Changed 9 years ago by tschaub

  • Resolution fixed deleted
  • State changed from Review to Needs more work
  • Status changed from closed to reopened

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

comment:11 Changed 9 years ago by sbenthall

  • Owner set to sbenthall
  • Status changed from reopened to new

comment:12 Changed 9 years ago by sbenthall

  • Status changed from new to assigned

comment:13 Changed 9 years ago by sbenthall

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).

comment:14 Changed 9 years ago by sbenthall

  • Component changed from widgets.MapPanel to widgets.Popup

comment:15 Changed 9 years ago by sbenthall

  • Owner changed from sbenthall to tschaub
  • State changed from Needs more work to Review
  • Status changed from assigned to new

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.

Changed 9 years ago by sbenthall

updated IE patch

comment:16 Changed 9 years ago by sbenthall

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

comment:17 Changed 9 years ago by sbenthall

  • Resolution set to fixed
  • Status changed from new to closed

(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)

Note: See TracTickets for help on using tickets.