Closed Bug 1141665 Opened 5 years ago Closed 5 years ago

Add vertical divider between tabs and devices in split pane panel design on Tablets

Categories

(Firefox for Android :: General, defect)

x86
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox37 --- unaffected
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: antlam, Assigned: vivek)

References

Details

Attachments

(2 files, 2 obsolete files)

> I think we're missing the vertical divider from the mock in comment 9. Can
> we add that in?

Wanted to file this bug since we closed the other. 

The equal height on items are really nice. But let's get that vertical divider back in :) it really helps separate the content.
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
Attached patch 1141665.patch (obsolete) — Splinter Review
Added a vertical separator between tabs and client list view.
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
Attachment #8575602 - Flags: review?(nalexander)
Attached patch 1141665.patch (obsolete) — Splinter Review
Using match_parent instead of fill_parent
Attachment #8575602 - Attachment is obsolete: true
Attachment #8575602 - Flags: review?(nalexander)
Attachment #8575604 - Flags: review?(nalexander)
Thanks Vivek!
Comment on attachment 8575604 [details] [diff] [review]
1141665.patch

This will work, but why 2dp and where is the color from?  antlam, is this what you want?

I see an example at [1].

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/overlay_share_toast.xml#46
Flags: needinfo?(alam)
Attachment #8575604 - Flags: review?(nalexander) → review+
I didn't see a screenshot yet. Vivek, could you post one?

The color should be #D7D9DB, like our other dividers.
Flags: needinfo?(alam) → needinfo?(vivekb.balakrishnan)
Attached patch 1141665.patchSplinter Review
Updated patch as per antlam's comment
Screenshot: https://www.dropbox.com/s/n5f9wxhbg2wdsqy/screenshot_final.png?dl=0
Attachment #8575604 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8575674 - Flags: review?(nalexander)
Comment on attachment 8575674 [details] [diff] [review]
1141665.patch

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

Looks good!
Attachment #8575674 - Flags: review?(nalexander) → review+
Assignee: nobody → vivekb.balakrishnan
Status: NEW → ASSIGNED
Comment on attachment 8575674 [details] [diff] [review]
1141665.patch

Approval Request Comment
[Feature/regressing bug #]: follow-up to Bug 1129171.
[User impact if declined]: ugliness?
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: very low.
[String/UUID change made/needed]: none.
Attachment #8575674 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/72bf69607e72
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Marking as affected starting in 38 based on information from bug 1129171.
Comment on attachment 8575674 [details] [diff] [review]
1141665.patch

Approving for uplift to 38 since this need to ride along with 1129171 and it sounds low-risk.
Attachment #8575674 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.