Closed Bug 1141665 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
x86
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox37 unaffected, firefox38+ fixed, firefox39 fixed)

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

People

(Reporter: antlam, Assigned: vivek)

References

Details

Attachments

(2 files, 2 obsolete files)

prev_tab_syncpanel_landscape2.png
223.72 KB, image/png
Details
1141665.patch
1.31 KB, patch
nalexander
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
> 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.patch — — Splinter 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: 8 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Marking as affected starting in 38 based on information from bug 1129171.
status-firefox37: --- → unaffected
status-firefox38: --- → affected
tracking-firefox38: --- → +
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+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.