769446 - "add a comment" link gets pushed off screen when "open reftest analyzer" is displayed

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Matt Brubeck (:mbrubeck)]

Maybe we can move the reftest analyzer link to the right-hand side of the footer?

Comment 1

[Edmund Wong (:ewong)]

Attachment: Moved "open reftest analyzer" to the right, above the duration display. (v1)

Comment 2

[Edmund Wong (:ewong)]

Attachment: Moved "open reftest analyzer" to the right, above the duration display. (v2)

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 654966 [details] [diff] [review]
Moved "open reftest analyzer" to the right, above the duration display. (v2)

Works for me.  Seeking a second opinion in case there's anything I haven't thought of.

Comment 4

[Ed Morley [:emorley]]

Comment on attachment 654966 [details] [diff] [review]
Moved "open reftest analyzer" to the right, above the duration display. (v2)

Review of attachment 654966 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this :-)
Does the job locally - am giving r+ - please can if you could upload an updated patch with these few bits addressed and I'll land it.

::: css/style.css
@@ +476,5 @@
> +.reftestanalyzer {
> +  position: absolute;
> +  right: 5px;
> +  bottom: 20px;
> +}

To be consistent with the rest of the stylesheet, I think this should be .reftestAnalyzer.

To make the link line up with 'add a comment' to the left, this needs to be:
bottom: 26px;

::: js/UserInterface.js
@@ +1509,4 @@
>            '                   function() { $("p").removeClass("loading").text("Fetching reftest log failed.") },\n' +
>            '                   function() { $("p").removeClass("loading").text("Fetching reftest log timed out.") });\n' +
>            '</script>';
> +        return '<span class="reftestanalyzer"><a href="data:text/html,' + escape(html) + '">open reftest analyzer</a></span>';

Whilst this works as-is, I think the code might be more readable, if the order was changed to match where they appear in the UI. ie: the whole htmlForReftests function should go after htmlForAddComment() and before htmlForDuration().

Comment 5

[Edmund Wong (:ewong)]

Attachment: Moved "open reftest analyzer" to the right, above the duration display (v3)

Fixed nits.

Comment 6

[Ed Morley [:emorley]]

I had to back this out and reland, since I didn't spot there wasn't an author set in the patch (since the previous patches in other bugs had it filled out). Please can you adjust your hgrc so this is added automatically :-)
https://developer.mozilla.org/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Landed as:
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/3631325e489e