592708 - Hide the bottom border of the details grid

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

The bottom-most visible row in the grid should not have a bottom border. This either needs javascript cleverness or maybe we can re-arrange the grid so the top-most row or bottom-most row are always visible.

Comment 1

[Roland Warmerdam]

Attachment: Removes last bottom border of detail-grid

Fix changes the CSS to remove the bottom border and has been tested in Linux.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 474008 [details] [diff] [review]
Removes last bottom border of detail-grid

What do you think Blair, I'm concerned that we'll hit a case where an add-on has no updated date but does have following items in the grid. Right now though the only add-on type without an update date is personas and they show nothing else either.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 474008 [details] [diff] [review]
Removes last bottom border of detail-grid

(In reply to comment #2)
> What do you think Blair, I'm concerned that we'll hit a case where an add-on
> has no updated date but does have following items in the grid. Right now though
> the only add-on type without an update date is personas and they show nothing
> else either.

Yea, that's a big enough issue, I think. It may be mostly fine right now, but it'll break as soon as those conditions change. And the UI does need to support different addon types, which can make that scenario more likely too.

As far as I know, there's no way to do this purely in CSS. There needs to be some attribute set ("last-row"?) either on the first visible or last visible row.

Comment 4

[Roland Warmerdam]

Attachment: Adds a first-row attribute and removes bottom border

Added some javascript cleverness to add a 'first-row' attribute to the first visible row in the grid, this should be fairly future proof. ;)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 474505 [details] [diff] [review]
Adds a first-row attribute and removes bottom border

Looks good! Just needs a couple of small code-style changes, to make the style fit in with the rest of the code in that file. See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide for reference.

>+    for ( var i=0,first=true; i < gridRows.length; ++i ) {

Remove the spaces inside the brackets, but add spaces on either side of the = and after the comma.
ie: for (var i = 0, first = true; ...) {

>+      if ( window.getComputedStyle(gridRows[i],null).getPropertyValue("display") != "none" && first ) {

Again, remove the spaces inside the brackets, but add one after the comma. Also, try to make it wrap/break up long lines like this, so lines are only 80 characters long.

It doesn't matter greatly here, but in general, putting the check of the "first" variable before the call to getComputedStyle() will mean getComputedStyle() won't need to be called if first is false. Checking whether "first" is true is a lot less computationally expensive than looking up the computed style.

>+      } else
>+        gridRows[i].removeAttribute("first-row");

Add brackets here. If the first part of an if-else block is surrounded in brackets, the second part should be too.

Comment 6

[Roland Warmerdam]

Attachment: Fixed javascript code formatting

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 476634 [details] [diff] [review]
Fixed javascript code formatting

Great - thanks!

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 476634 [details] [diff] [review]
Fixed javascript code formatting

Approved to land after b7

Comment 9

[Dave Townsend [:mossop]]

Landed, thanks for the patch: http://hg.mozilla.org/mozilla-central/rev/5a25bf78ba5d

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> Landed, thanks for the patch:
> http://hg.mozilla.org/mozilla-central/rev/5a25bf78ba5d

So this patch should be part of all platforms but I cannot see a difference between the latest b7pre nightly build from 101006 and todays b8pre nightly build on OS X. Something I miss here?

Comment 11

[Dave Townsend [:mossop]]

It WFM on the current OSX nightly, can you post a screenshot of what you're seeing?

Comment 12

[Roland Warmerdam]

I just pulled down the latest changes and I can't find any trace of the patch changes in the code.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> I just pulled down the latest changes and I can't find any trace of the patch
> changes in the code.

Seems to be there for me: http://hg.mozilla.org/mozilla-central/annotate/tip/toolkit/mozapps/extensions/content/extensions.js#l2013

Comment 14

[Roland Warmerdam]

(In reply to comment #13)
> (In reply to comment #12)
> > I just pulled down the latest changes and I can't find any trace of the patch
> > changes in the code.
> 
> Seems to be there for me:
> http://hg.mozilla.org/mozilla-central/annotate/tip/toolkit/mozapps/extensions/content/extensions.js#l2013

I updated again today and now the change is there.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #11)
> It WFM on the current OSX nightly, can you post a screenshot of what you're
> seeing?

No matter. Was my fault. Looks fine with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101010 Firefox/4.0b8pre.