Closed Bug 592708 Opened 9 years ago Closed 9 years ago

Hide the bottom border of the details grid

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- -

People

(Reporter: mossop, Assigned: rowno)

Details

(Keywords: polish)

Attachments

(1 file, 2 obsolete files)

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.
blocking2.0: --- → final+
Fix changes the CSS to remove the bottom border and has been tested in Linux.
Assignee: nobody → rowno
Status: NEW → ASSIGNED
Attachment #474008 - Flags: review?(dtownsend)
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.
Attachment #474008 - Flags: review?(dtownsend) → review?(bmcbride)
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.
Attachment #474008 - Flags: review?(bmcbride) → review-
Added some javascript cleverness to add a 'first-row' attribute to the first visible row in the grid, this should be fairly future proof. ;)
Attachment #474008 - Attachment is obsolete: true
Attachment #474505 - Flags: review?(bmcbride)
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.
Attachment #474505 - Flags: review?(bmcbride) → review-
Attachment #474505 - Attachment is obsolete: true
Attachment #476634 - Flags: review?(bmcbride)
blocking2.0: final+ → -
Keywords: polish
Comment on attachment 476634 [details] [diff] [review]
Fixed javascript code formatting

Great - thanks!
Attachment #476634 - Flags: review?(bmcbride) → review+
Comment on attachment 476634 [details] [diff] [review]
Fixed javascript code formatting

Approved to land after b7
Attachment #476634 - Flags: approval2.0? → approval2.0+
Whiteboard: [good first bug] → [has patch][checkin-needed-post-b7]
Landed, thanks for the patch: http://hg.mozilla.org/mozilla-central/rev/5a25bf78ba5d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin-needed-post-b7]
Target Milestone: --- → mozilla2.0b8
(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?
It WFM on the current OSX nightly, can you post a screenshot of what you're seeing?
I just pulled down the latest changes and I can't find any trace of the patch changes in the code.
(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
(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.
(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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Flags: in-litmus-
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.