Designs to come soon

Putting an initial stake in the ground for people to start roughing in the the tablet UI. Sketches: http://cl.ly/image/3S3H1J1S3x0D Notable differences from phone: * Larger thumbnails * 3x3 thumbnail grid (instead of 2x3) * 2 column reading list (implement after we land the more visual reading list previews, bug 889351) * Side tabs for sections in the 'Visited' panel I'll post some more refined designs soon, but we can start by reusing the basic styling we've created for the phone UI

Ok, visual designs for the tablet UI can be found here: http://cl.ly/1z3l0g2D1E17 As you can see most of the styles are inherited from the phone UI, so the majority of the changes are structural / tweaks to margins and whatnot.

And, another pack of mockups here, this time with margins measured out: http://cl.ly/0K0Y050q0S2y

This adds changes for the BookmarksPage. This gets the values right as per the mockups. However, in the long run, I feel, the code maintenance will be a problem. Here are the problems I faced/question I had when translating the design: 1. The top margin of 30dp -- is it for the entire page? or just the top-boomarks? (This can be changed as needed, may be in a followup). 2. If there's a top margin of 30dp, is there a bottom margin too? Or the list bleeds through the edge of the tablet? 3. I'm still confused about the horizontal and vertical spaces to be used in each thumbnail view. 4. The leftmost row's thumbnail's left border should align with the favicons in the list. However, the thumbnail have a 5dp padding to show the highlighted state. Does this padding amount change in tablets? Do we need to account for this? (So an inset of 60dp for thumbnails will actually translate to 55dp of padding and a 5dp of highlight area -- XML will be filled with magic calculations and code maintenance is a problem). 5. It so happens that the height of top-thumbnails section == height of tablet (accounting for browser-toolbar). This makes the list not even seen. Users might not even know that there is a list below the thumbnails. Implementation notes: 1. getHorizontalPadding() is not available in API level 8. Hence using it through the TypedArray. 2. homeListViewStyle is common for all lists, and a padding cannot be applied directly to it. Hence created a new styleable bookmarksListViewStyle. 3. home_item_row is a generic too. Hence created bookmark_item_row.

(In reply to Sriram Ramasubramanian [:sriram] from comment #4) > Created attachment 785114 [details] [diff] [review] > Part 1: Bookmarks > > This adds changes for the BookmarksPage. This gets the values right as per > the mockups. However, in the long run, I feel, the code maintenance will be > a problem. Here are the problems I faced/question I had when translating the > design: > 1. The top margin of 30dp -- is it for the entire page? or just the > top-boomarks? (This can be changed as needed, may be in a followup). > 2. If there's a top margin of 30dp, is there a bottom margin too? Or the > list bleeds through the edge of the tablet? Also, if the margin is going with the page, what the space after the last row of bookmarks? If we don't give a space there, the bookmarks will sit very close to the lists. > 4. The leftmost row's thumbnail's left border should align with the favicons > in the list. However, the thumbnail have a 5dp padding to show the > highlighted state. Does this padding amount change in tablets? Do we need to > account for this? (So an inset of 60dp for thumbnails will actually > translate to 55dp of padding and a 5dp of highlight area -- XML will be > filled with magic calculations and code maintenance is a problem). Also, in phones, the padding for the list is 7dp, and a 5dp is applied for highlighting area -- making it 12dp from leftmost edge. The favicon is at 10dp. Do we need changes there too?

(In reply to Sriram Ramasubramanian [:sriram] from comment #4) > This adds changes for the BookmarksPage. This gets the values right as per > the mockups. However, in the long run, I feel, the code maintenance will be > a problem. Here are the problems I faced/question I had when translating the > design: > 1. The top margin of 30dp -- is it for the entire page? or just the > top-boomarks? (This can be changed as needed, may be in a followup). Entire page > 2. If there's a top margin of 30dp, is there a bottom margin too? Or the > list bleeds through the edge of the tablet? Yep, there should be a bottom margin too. I would suggest maybe even more than 30. Possibly 60, to make the layout feel a little more open. > 3. I'm still confused about the horizontal and vertical spaces to be used in > each thumbnail view. Let's talk on IRC tomorrow. > 4. The leftmost row's thumbnail's left border should align with the favicons > in the list. However, the thumbnail have a 5dp padding to show the > highlighted state. Does this padding amount change in tablets? Do we need to > account for this? (So an inset of 60dp for thumbnails will actually > translate to 55dp of padding and a 5dp of highlight area -- XML will be > filled with magic calculations and code maintenance is a problem). One way or another, the thumbnail edges need to align to the favicons in the list. But again, let's dive into this a little more on IRC tomorrow so I can better understand the issues you're having. > 5. It so happens that the height of top-thumbnails section == height of > tablet (accounting for browser-toolbar). This makes the list not even seen. > Users might not even know that there is a list below the thumbnails. That's not good. We want to look for any way we can to communicate this view is scrollable. Please post some screenshots at a small tablet size (like a nexus 7) and a large tablet size (like a nexus 10). We may need to fiddle around with some spacing to see if we can make it more apparent that the page is scrollable. We'll still run into this on odd sized devices, though, so we should also start thinking about a better long term solution...

Comment on attachment 785114 [details] [diff] [review] Part 1: Bookmarks I need to catch up on the new About:Home code. Lucas is better suited for this review.

Comment on attachment 785114 [details] [diff] [review] Part 1: Bookmarks Review of attachment 785114 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I assume this still needs to go through ibarlow's review?

I'll try a test build whenever you're ready.

Bookmarks page looks great. Only issue I see is that on smaller tablets, rotating from portrait to landscape causes the thumbnails to go tiny. Fix that and we are good to go.

Oops. I had missed the 7" on landscape. But then I had the 10" values to be used for 7" too. Hence it had 56dp padding. This patch moves the 10" values to xlarge-land-v11 and changes the horizontal spacing on 7". I can fold this with the other patch and land together.

Comment on attachment 786934 [details] [diff] [review] Part 2: Bookmarks 7" Review of attachment 786934 [details] [diff] [review]: ----------------------------------------------------------------- Nice. I'll assume ibarlow has reviewed this change too.

Folded and pushed as a single patch (Part 1 & 2): http://hg.mozilla.org/projects/fig/rev/23affad4e7a1

That little minimal small tiny change needed for Reading lists on tablets.

Oh! didn't make the change for the rows in previous patch. This takes care of it. And please don't worry about re-using the bookmark_item_row. :) They both are currently the same and in the next version of reading-list, reading-list will get a new file for itself.

Mockups wants a top divider (divider on top of the list) for Reading list and history pages (not bookmarks as there are thumbnails). This patches adds an attribute to XML to do the same. This can be re-used with any list extending HomeListView -- hence by history lists too. Note: ListView always draws a bottom dividers if the height of the rows is less than the max-height of the ListView. When there are more entries (when it actually scrolls), we don't need a bottom most one -- as its merges with the tablet's boundary.

Comment on attachment 788307 [details] [diff] [review] Part 3: Reading list Review of attachment 788307 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. IIRC, ibarlow has said he wanted to try using the phone UI on small tablets. This patch doesn't seem to account for that. Just checking: is this something that you and ibarlow has explicitly agreed on? ::: mobile/android/base/resources/values-large-v11/styles.xml @@ +55,5 @@ > <item name="android:verticalSpacing">10dp</item> > </style> > > + <style name="Widget.ReadingListView" parent="Widget.BookmarksListView"> > + <item name="android:paddingTop">30dp</item> Is it 30dp independent of orientation? Even, say, on small tablets in portrait?

Comment on attachment 788325 [details] [diff] [review] Part 5: Top divider Review of attachment 788325 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: mobile/android/base/home/HomeListView.java @@ +37,5 @@ > // On URL open listener > private OnUrlOpenListener mUrlOpenListener; > > + // Top divider > + private boolean mTopDivider; mShowTopDivider? @@ +66,5 @@ > + if (mTopDivider && divider != null) { > + final int dividerHeight = getDividerHeight(); > + final View view = new View(getContext()); > + view.setLayoutParams(new LayoutParams(LayoutParams.FILL_PARENT, dividerHeight)); > + addHeaderView(view); I assume this doesn't cause any regressions on the bookmarks page (with the grid header view and all)? Please double-check that.

Comment on attachment 788316 [details] [diff] [review] Part 4: Reading list row Review of attachment 788316 [details] [diff] [review]: ----------------------------------------------------------------- This is just to make reading list rows follow the same style than bookmark's, right? Just wondering: what's the point of keeping home_item_row then? It just seems like it would be better to simply apply the style changes directly to home_item_row instead. Just want some clarification before approving this.

(In reply to Lucas Rocha (:lucasr) from comment #19) > Comment on attachment 788316 [details] [diff] [review] > Part 4: Reading list row > > Review of attachment 788316 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is just to make reading list rows follow the same style than > bookmark's, right? Just wondering: what's the point of keeping home_item_row > then? It just seems like it would be better to simply apply the style > changes directly to home_item_row instead. Just want some clarification > before approving this. No. home_item_row is being used by history tab pages too. This change was made because, the padding on left and right on the rows are same for bookmarks and reading list pages. However, in case of history pages it is different. Hence we can have just one home_item_row.

(In reply to Lucas Rocha (:lucasr) from comment #17) > Comment on attachment 788307 [details] [diff] [review] > Part 3: Reading list > > Review of attachment 788307 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. IIRC, ibarlow has said he wanted to try using the phone UI on > small tablets. This patch doesn't seem to account for that. Just checking: > is this something that you and ibarlow has explicitly agreed on? > > ::: mobile/android/base/resources/values-large-v11/styles.xml > @@ +55,5 @@ > > <item name="android:verticalSpacing">10dp</item> > > </style> > > > > + <style name="Widget.ReadingListView" parent="Widget.BookmarksListView"> > > + <item name="android:paddingTop">30dp</item> > > Is it 30dp independent of orientation? Even, say, on small tablets in > portrait? Yes. It's 30dp everywhere.

(In reply to Lucas Rocha (:lucasr) from comment #18) > Comment on attachment 788325 [details] [diff] [review] > Part 5: Top divider > > Review of attachment 788325 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. > > ::: mobile/android/base/home/HomeListView.java > @@ +37,5 @@ > > // On URL open listener > > private OnUrlOpenListener mUrlOpenListener; > > > > + // Top divider > > + private boolean mTopDivider; > > mShowTopDivider? > > @@ +66,5 @@ > > + if (mTopDivider && divider != null) { > > + final int dividerHeight = getDividerHeight(); > > + final View view = new View(getContext()); > > + view.setLayoutParams(new LayoutParams(LayoutParams.FILL_PARENT, dividerHeight)); > > + addHeaderView(view); > > I assume this doesn't cause any regressions on the bookmarks page (with the > grid header view and all)? Please double-check that. This won't be applied for the BookmarksPage. I've tested it.

http://hg.mozilla.org/projects/fig/rev/20ae8ca10179 http://hg.mozilla.org/projects/fig/rev/0de8644cd3e2 http://hg.mozilla.org/projects/fig/rev/6cb89142d14b

https://hg.mozilla.org/mozilla-central/rev/23affad4e7a1 https://hg.mozilla.org/mozilla-central/rev/20ae8ca10179 https://hg.mozilla.org/mozilla-central/rev/0de8644cd3e2 https://hg.mozilla.org/mozilla-central/rev/6cb89142d14b