Opened 9 years ago

Closed 6 years ago

#5 closed enhancement (fixed)

Flexible popup positioning

Reported by: sbenthall Owned by: sbenthall
Priority: minor Milestone: 1.1
Component: GeoExt.Popup Version:
Keywords: popups Cc:
State: Complete

Description

http://geoext.org/svn/geoext/sandbox/sbenthall/popup/lib/GeoExt/widgets/popup/

Currently, anchored popups can only appear above a feature. But this inflexibility causes problems when, for example, a feature is very close to the top of the map (so the map cannot pan the popup into view).

Should build in support for popups that appear on either side and below the feature they are anchored to.

Attachments (6)

patch-flexible-popup-positioning-5-A0.patch (10.1 KB) - added by chrismayer 7 years ago.
anchor-bottom.png (638 bytes) - added by chrismayer 7 years ago.
Needed anchor image for patch
anchor-top.png (638 bytes) - added by ahocevar 7 years ago.
calling it anchor-top because the anchor is at the top of the window
geoext-5.patch (9.0 KB) - added by ahocevar 7 years ago.
anchor-top.2.png (380 bytes) - added by chrismayer 7 years ago.
The grey anchor for the top position
geoext-5.1.patch (15.6 KB) - added by chrismayer 6 years ago.
Patch providing multiple positioning without the resize zone issue

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by sbenthall

  • Keywords popups added

comment:2 Changed 9 years ago by rpenate

Something like the upper half of this quick mockup, for example: http://img.skitch.com/20090115-trc457jb142buqydc5uee4gap3.png

comment:3 Changed 8 years ago by tschaub

  • Milestone set to 0.6

comment:4 Changed 8 years ago by tschaub

  • Milestone changed from 0.6 to 0.7

comment:5 Changed 8 years ago by fredj

  • Component changed from GeoExt.data to GeoExt.Popup

comment:6 Changed 8 years ago by elemoine

  • Milestone changed from 0.7 to 1.0

comment:7 Changed 7 years ago by ahocevar

  • Milestone changed from 1.0 to 1.1

No patch. Bumping.

Changed 7 years ago by chrismayer

Changed 7 years ago by chrismayer

Needed anchor image for patch

comment:8 Changed 7 years ago by chrismayer

Hello,

I wrote a patch regarding this ticket:

Now it is possible to set a flag if it should be decided automatically if the popup has to be rendered on top or below its location. At the moment the decision criteria is if the location is below or up the half of the map height. Hope this helps the project.

I would be thankful for any review.

Best regards, Chris

Changed 7 years ago by ahocevar

calling it anchor-top because the anchor is at the top of the window

Changed 7 years ago by ahocevar

comment:9 Changed 7 years ago by ahocevar

Hey Chris, thanks for the patch. I have updated it a bit:

  • use anchorPosition instead of onTop and renderBottomTop
  • use a separate css class for the top anchor
  • renamed the image to anchor-top.png
  • made automatic positioning the default
  • coding style improvements (line breaks)

I have not updated the example, and have only done visual testing on Safari5.

The patch still needs more work:

  • Something is wrong with the resize zones at the window borders. Even when the anchor is rendered on the top edge of the window, there is a resize zone below the poppup (hover over the bottom edge of the popup, and move the mouse down. Watch the cursor change back to normal way below the border).
  • The image for the grey theme is missing.
  • The ticket suggests anchor positioning on either side (i.e. on the right-top and right-bottom) as well.

comment:10 Changed 7 years ago by chrismayer

Hi Andreas, hi devs,

thanks for reviewing the patch. I can provide the missing features for positioning on either side (i.e. on the right-top and right-bottom). The image for the grey theme is also no problem. But the thing with the resize zones is not that simple:

IMHO the behaviour is also wrong in the stable trunk version of the Popup class (Anchor at the bottom). Here the resize zone is also too large at the bottom of the window. The reason is the DIV which is added for the anchor. Its initial height is the size of the buffer enlarging the resize zone at the bottom of the window. I tried some different ways to add the anchor to the window but without success so far. How to proceed now? Provide a patch for the positioning stuff mentioned above or should the resize zone problems be solved first?

Thanks, Chris

comment:11 Changed 7 years ago by ahocevar

Thanks Chris for your ongoing effort. I'd say it's okay to create a separate ticket for the resize zone issues, but I think the solution will be to position the anchor as separate floating element, rather than making it part of the window frame. So please go ahead and provide whatever you can, and we can look at the resize zone problems later.

Changed 7 years ago by chrismayer

The grey anchor for the top position

Changed 6 years ago by chrismayer

Patch providing multiple positioning without the resize zone issue

comment:12 Changed 6 years ago by chrismayer

Hi Andreas, hi devs,

I attached a patch including the missing features for positioning on each side (top-right, etc.). Also tests and an example are included. This patch can be applied stand-alone, only the image has to be copied.

As discussed, I will open another ticket regarding the resize zone issues.

Best regards, Chris

comment:13 Changed 6 years ago by chrismayer

  • State changed from Needs more work to Review

comment:14 Changed 6 years ago by ahocevar

  • State changed from Review to Needs more work

Hey Chris, thanks for the continued work on this. I think that the resize zone issue can easily be fixed by using position: absolute instead of position: relative for the .gx-popup-anc selector. And I think it makes sense to address this as part of this ticket, because it will change the way you position the anchor for all of the four positions.

Other remarks:

  • You append 'px' to values you pass to Ext.Element::setWidth/setHeight. This violates the api - just remove it.
  • You use things like width-10 to position the anchor on the right. If you look at the .gx-popup-anc selector, you see that it has a left: 5px style set. So this limits the freedom of people who want to customize the anchor position with css. The proper way to address this would be to have a .right selector, like the .top selector you already have, which sets left: inherit and right: 5px, and not hard-code offsets in javascript.
  • Please always use 4 spaces to indent, no tabs.

comment:15 Changed 6 years ago by ahocevar

left: inherit above should be replaced with left: 0.

comment:16 Changed 6 years ago by ahocevar

Another remark: Wherever possible, assign values you get from functions like getWidth() to local variables if you use them more than once. Not only will it make things a bit faster, it will also allow js compressors to do a better job.

comment:18 Changed 6 years ago by fredj

  • Resolution set to fixed
  • State changed from Needs more work to Complete
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.