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

Proposed by Deryck Hodge
Status: Merged
Approved by: Eleanor Berger
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
Reviewer Review Type Date Requested Status
Eleanor Berger (community) code ui Approve
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

To post a comment you must log in.
Revision history for this message
Eleanor Berger (intellectronica) wrote :

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 :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.