Launchpad itself

Merge lp://staging/~deryck/launchpad/pop-up-help-positioning-574682 into lp://staging/launchpad

pop-up-help-positioning-574682
Merge into devel

Proposed by Deryck Hodge on 2010-05-08

Status:

Merged

Approved by:

Eleanor Berger on 2010-05-10

Approved revision:

no longer in the source branch.

Merged at revision:

10844

Proposed branch:

lp://staging/~deryck/launchpad/pop-up-help-positioning-574682

Merge into:

lp://staging/launchpad

Diff against target:

11 lines (+2/-0)

1 file modified

lib/lp/services/inlinehelp/javascript/inlinehelp.js (+2/-0)

To merge this branch:

bzr merge lp://staging/~deryck/launchpad/pop-up-help-positioning-574682

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Eleanor Berger (community)	code ui	2010-05-08	Approve on 2010-05-10
Review via email: mp+24943@code.staging.launchpad.net

Commit message

Fix positioning of pop-up help overlays.

Description of the change

This fixes bug 574682. It makes pop-up help overlays consider the viewport position as part of the setElementPosition call. Currently on Launchpad, pop-up help is positioned off screen if you scroll down the page and then select the help icon (on tags, for example).

I have no idea how to test this since it's MochiKit-based. I'm happy to learn and write a test if we have anything for MochiKit-based code.

Cheers,
deryck

Revision history for this message

Eleanor Berger (intellectronica) wrote on 2010-05-10:

Thanks for fixing this annoying bug. I don't think it's necessary to add a test (you could use Windmill if you really wanted), but I wonder if any consideration has been given to converting this to use LAZR-JS. I very vaguely remember Maris talking about it at some point. Maybe it's worth letting him know that we're now fixing bugs in this legacy code.

The fix looks fine and works well. You can remove the if statement. It is not necessary and I don't think aids readability. Up to you.

review: Approve (code ui)

Revision history for this message

Deryck Hodge (deryck) wrote on 2010-05-10:

Hi, Tom.

Thanks for the review.

I looked into converting to lazr-js myself, but I didn't have time to invest when a simple fix will work. There are still other, deeper dependencies on MochiKit than this bit of code, so it didn't seem worth it to me for now. I don't think it's worth a Windmill test, either, to be honest.

I did remove the if statement. I thought it was a bit more clear why this was needed with the if statement, but no harm without either, so I dropped it.

Thanks again for the review today.

Cheers,
deryck