Closed Bug 894077 Opened 11 years ago Closed 11 years ago

[FIG] Tablet layout for visited page

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Theme and Visual Design
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 26

People

(Reporter: shilpanbhagat, Assigned: shilpanbhagat)

References

Details

(Whiteboard: abouthome-hackathon, fixed-fig)

Attachments

(5 files, 10 obsolete files)

7" landscape
62.33 KB, image/png
Details
7" portrait
62.15 KB, image/png
Details
10" landscape
101.47 KB, image/png
Details
10" portrait
101.79 KB, image/png
Details
Patch: Layout for tablet, history page
45.01 KB, patch
shilpanbhagat
: review+
Details | Diff | Splinter Review
Redesign the visited page on about:home based on the design: http://cl.ly/image/3S3H1J1S3x0D/o
This first patch changes the layout according to the proposed changes. It still needs work with respect to how the layout is laid out based on orientation change.
Attachment #775975 - Flags: feedback?(lucasr.at.mozilla)
Assignee: nobody → sbhagat
Attached image Screenshot of visited page (obsolete) —
This build contains the new visited page: https://dl.dropboxusercontent.com/u/11916346/tablet_test_build.apk PS: use it on a tablet.
Need specific designs
Flags: needinfo?(ibarlow)
Comment on attachment 775975 [details] [diff] [review] [WIP] Changing layout for visited page Review of attachment 775975 [details] [diff] [review]: ----------------------------------------------------------------- The direction is overall good but we need to figure out some big picture issues before going ahead with this approach. When I wrote the subpage system, I had a very specific goal and some important assumptions in mind. The subpages on phones cover the whole content area, which means that you cannot switch to another homepager page while viewing them. This allowed us to handle destruction of subpage high up in BrowserApp because the assumption is that the subpage is destroyed either by hiding the homepager or by dismissing them with the back button. Furthermore, we could work with the assumption that there's only one container where you can place subpages (the home_pager_container view). The container's lifecycle is tied to the main activity which means that it will always live longer the fragments it contains. The same assumptions do not necessarily apply on tablets though. The UI will allow switching pages even while subpages are being displayed. This means we might end up leaking fragments while switching pages in homepager (note how we use the activity's FragmentManager to add subpages). Also, the lifecycle of the subpage container on tablets is tied to the lifecycle of the VisitedPageLayout fragment which might be destroyed before the subpage fragments (e.g. when you switch to another page in homepager). This might cause undefined behavior in many situations. Given the new requirements for the tablet UI, I can see two options: 1. Use nested fragments in the Visited tab. This means getting rid of the current subpage system and simply relying on the nested FragmentManager to manage the subpage lifecycle. 2. Turn each subpage into a custom view and manage their respective loaders in their parent fragment. This is roughly the approach that sriram took in the BookmarksPage (for the grid and list of bookmarks). There will probably be some extra work to make back stack work properly without relying on the fragment manager. The advantage of option 1 is that it's probably simpler to implement as it involves less changes in the current code. On the other hand, option 1 involves nesting fragments which is a potential can of worms. The main advantage of option 2 is that we can avoid the use of nested fragments entirely. But it will probably involve a lot more changes in terms of turning fragments into custom views. I'm personally leaning towards options 2. But I'm open for feedback on this before deciding. ::: mobile/android/base/home/HistoryPage.java @@ +108,5 @@ > > @Override > public void onViewCreated(View view, Bundle savedInstanceState) { > final TextView title = (TextView) view.findViewById(R.id.title); > + if (title != null) { Same here. This change chould probably go in a separate patch. ::: mobile/android/base/home/LastTabsPage.java @@ +100,5 @@ > > @Override > public void onViewCreated(View view, Bundle savedInstanceState) { > final TextView title = (TextView) view.findViewById(R.id.title); > + if (title != null) { This change chould probably go in a separate patch. ::: mobile/android/base/resources/drawable/tablet_visited_page_button.xml @@ +1,1 @@ > +<?xml version="1.0" encoding="utf-8"?> You can probably call this home_page_button.xml. I wonder if you could simply use it in the phone's visited page buttons too? ::: mobile/android/base/resources/layout/home_list_with_title.xml @@ +16,5 @@ > + style="@style/AboutHome.EmptyMessage" > + android:layout_width="wrap_content" > + android:layout_height="wrap_content" > + android:text="@string/abouthome_visited_empty" > + android:visibility="gone"/> Why is this necessary in this patch? ::: mobile/android/base/resources/layout/home_visited_page.xml @@ +19,5 @@ > <LinearLayout android:id="@+id/buttons_container" > android:layout_width="fill_parent" > android:layout_height="wrap_content" > android:orientation="vertical" > + android:background="@color/background_light"> Why is the visibility attribute gone here? This was added to avoid flashing the buttons when history list is empty.
Attachment #775975 - Flags: feedback?(lucasr.at.mozilla)
Blocks: new-about-home
Priority: -- → P1
Blocks: 895837
Blocks: 888905
Whiteboard: abouthome-hackathon
> The subpages on phones cover the whole content area, which means that you cannot switch to another homepager page while viewing them. This allowed us to handle destruction of subpage high up in BrowserApp because the assumption is that the subpage is destroyed either by hiding the homepager or by dismissing them with the back button. I think we should/are throwing away this assumption now (although I still don't really like having tabs inside tabs).
Made changes based on the new design for tablets for History panel
Attachment #775975 - Attachment is obsolete: true
Attachment #775982 - Attachment is obsolete: true
Attachment #785253 - Flags: feedback?(sriram)
Attached image Screenshot (obsolete) —
The screenshot shows how it looks currently.
Attached image Portrait screenshot (obsolete) —
Comment on attachment 785253 [details] [diff] [review] [WIP] Changing layout for history panel Review of attachment 785253 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Suggestions: 1. Move the tablet specific drawable to drawable-large-v11 2. Size of the text in the screenshot looks weird. Probably you should be re-using some existing styles to avoid it. 3. Please use styles instead of dimens. Dimens aren't needed for these layouts. ::: mobile/android/base/Makefile.in @@ +1075,5 @@ > res/drawable/favicon_bg.xml \ > res/drawable/handle_end_level.xml \ > res/drawable/handle_start_level.xml \ > res/drawable/home_history_tabs_indicator.xml \ > + res/drawable/home_history_tabs_indicator_tablet.xml \ This should be moved to drawable-large-v11/ folder. Please remove "_tablet". large-v11 will take care of it. ::: mobile/android/base/TabsPanel.java @@ -105,5 @@ > } > }); > > - ImageButton button; > - Resources resources = getContext().getResources(); Do we use Resources anywhere else? If not, might need to remove the imports too. @@ +108,5 @@ > mTabWidget = (IconTabWidget) findViewById(R.id.tab_widget); > > + mTabWidget.addTab(R.drawable.tabs_normal, R.string.tabs_normal); > + mTabWidget.addTab(R.drawable.tabs_private, R.string.tabs_private); > + mTabWidget.addTab(R.drawable.tabs_synced, R.string.tabs_synced); So clean! :) ::: mobile/android/base/home/HistoryPage.java @@ +81,5 @@ > + // is in the background, onConfigurationChanged() is fired. > + // onConfigurationChanged() is called before onResume(), so > + // using commit() would throw an IllegalStateException since it can't > + // be used between the Activity's onSaveInstanceState() and > + // onResume(). Make this comment shorter. "Rotation should detach and re-attach to use a different layout". ::: mobile/android/base/resources/layout-large-v11/home_history_tabs_indicator.xml @@ +8,5 @@ > + android:layout_height="wrap_content" > + android:paddingBottom="@dimen/toast_button_padding" > + android:paddingTop="@dimen/toast_button_padding" > + android:background="@drawable/home_history_tabs_indicator_tablet" > + style="@style/Widget.Home.PageTitle"/> Style should be second line after xmlns. That's what we tend to follow. ::: mobile/android/base/resources/layout-large-v11/home_list_with_title.xml @@ +21,5 @@ > + android:paddingRight="@dimen/home_history_list_padding_right" > + android:paddingTop="@dimen/home_histoy_tablet_top_padding" > + android:scrollbarStyle="outsideOverlay"/> > + > +</merge> Why is this file even needed? What's the difference between this and the phone version? If it's just padding and scrollbarStyle, please move it to styles and use one file. I don't see a reason for two files. ::: mobile/android/base/resources/layout/tabs_panel_indicator.xml @@ +8,5 @@ > + android:layout_height="@dimen/browser_toolbar_height" > + android:minWidth="@dimen/tabs_panel_indicator_width" > + android:paddingTop="@dimen/browser_toolbar_button_padding" > + android:paddingBottom="@dimen/browser_toolbar_button_padding" > + android:background="@drawable/tabs_panel_indicator"/> What has changed in this file? ::: mobile/android/base/resources/values-large-land-v11/dimens.xml @@ +9,5 @@ > + <dimen name="home_history_list_padding_right">100dp</dimen> > + <dimen name="home_history_tabs_length">360dp</dimen> > + <dimen name="home_history_tabs_padding">100dp</dimen> > + > +</resources> Use styles instead of dimens. dimens are good, if they are going to be re-used in many places. Like "browser_toolbar_height" (which is also needed in java code). Otherwise, styles are better. ::: mobile/android/base/resources/values/attrs.xml @@ +194,5 @@ > </declare-styleable> > > <declare-styleable name="IconTabWidget"> > <attr name="android:layout"/> > + <attr name="display"> A new line before this line. Probably a comment too, explaining what this attribute is for. ::: mobile/android/base/resources/values/dimens.xml @@ +77,5 @@ > + <dimen name="home_histoy_tablet_top_padding">30dp</dimen> > + <dimen name="home_history_list_padding_left">32dp</dimen> > + <dimen name="home_history_list_padding_right">32dp</dimen> > + <dimen name="home_history_tabs_length">270dp</dimen> > + <dimen name="home_history_tabs_padding">0dp</dimen> Move it to styles. ::: mobile/android/base/widget/IconTabWidget.java @@ +28,5 @@ > super(context, attrs); > > TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.IconTabWidget); > mButtonLayoutId = a.getResourceId(R.styleable.IconTabWidget_android_layout, 0); > + int type = a.getInt(R.styleable.IconTabWidget_display, 0x00); mIsIcon = (a.getInt(...) == 0); @@ +40,5 @@ > > + public void addTab(int imageResId, int stringResId) { > + View button = LayoutInflater.from(getContext()).inflate(mButtonLayoutId, this, false); > + if (mIsIcon) { > + ((ImageButton)button).setImageResource(imageResId); Space between (ImageButton) and button. @@ +42,5 @@ > + View button = LayoutInflater.from(getContext()).inflate(mButtonLayoutId, this, false); > + if (mIsIcon) { > + ((ImageButton)button).setImageResource(imageResId); > + } else { > + ((TextView)button).setText(getContext().getString(stringResId).toUpperCase()); Space between (TextView) and button. @@ +48,3 @@ > > addView(button); > + button.setContentDescription(getContext().getString(stringResId)); Content description is needed for ImageButton only. TextView defaults to the text shown. Move it inside the "if" clause.
Attachment #785253 - Flags: feedback?(sriram) → feedback+
This patch optimizes history page for tablets.
Attachment #785253 - Attachment is obsolete: true
Attachment #786777 - Flags: review?(sriram)
(In reply to Sriram Ramasubramanian [:sriram] from comment #9) > ::: mobile/android/base/TabsPanel.java > @@ -105,5 @@ > > } > > }); > > > > - ImageButton button; > > - Resources resources = getContext().getResources(); > > Do we use Resources anywhere else? If not, might need to remove the imports > too. We do use Resources elsewhere
(In reply to Shilpan Bhagat from comment #8) > Created attachment 785282 [details] > Portrait screenshot Hey shilpan, it looks like you are using the same absolute measurements that apply to larger tablets here on a smaller, 7" size which makes the left and right column widths look weird. The column widths should scale more responsively, so that the left column is much smaller both in landscape and portrait. A rough rule of thumb would be that the relative widths of tabs vs list should be 1:2 If that's too vague, let me know and I can mock up some small tablet specific about:home screens.
Flags: needinfo?(ibarlow)
Actually, I'm looking at a build on the Nexus 7, and I'd like to amend that previous comment a little. Small tablets Portrait mode: use the phone UI with tabs on bottom. It's a better use of UI space for that screen size and orientation. Landscape mode: use the tabs-on-side design Large tablets (~9"+) Portrait and Landscape mode: use the tabs-on-side design
Made changes according to Ian's comments.
Attachment #786777 - Attachment is obsolete: true
Attachment #786777 - Flags: review?(sriram)
Attachment #787322 - Flags: feedback?(sriram)
Oops, forgot to add the latest patch
Attachment #787322 - Attachment is obsolete: true
Attachment #787322 - Flags: feedback?(sriram)
Attachment #787327 - Flags: feedback?(sriram)
Comment on attachment 787327 [details] [diff] [review] Patch: Layout for tablet, history page Review of attachment 787327 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. But the way the styles are used should be changed. 1. The layout file can use a Widget.Home.ListView style. 2. This style shouldn't have any layout specific values. 3. Layout values should be available only in layout xml files. (Unless it's something like a fixed sized "ActionBarIcon"). 4. Needs some padding changes in tablet portrait mode. 5. The divider in the TabWidget should be of same size as the entire text. (Look for "dividerPadding" and set it to zarro). Good for feedback. Not review ready yet. ::: mobile/android/base/Makefile.in @@ +1085,5 @@ > res/drawable/favicon_bg.xml \ > res/drawable/handle_end_level.xml \ > res/drawable/handle_start_level.xml \ > res/drawable/home_history_tabs_indicator.xml \ > + res/drawable/home_history_tabs_indicator_tablet.xml \ Please move this to drawable-large-land-v11 (or drawable-large-v11). It's not good to have it here. ::: mobile/android/base/home/HistoryPage.java @@ +46,4 @@ > > + // Show most visited page as the initial page. > + // Since we detach/attach on config change, this prevents from replacing current fragment. > + if (!initializeVisitedPage) { Will this prevent us from showing MostVisitedPage in the following scenario: 1. I swipe to history page. 2. Select MostRecent tab. 3. Load a page. 4. Hit about:home -- now in bookmarks 5. Now back to History page -- will this show MostVisited? or MostRecent? @@ +80,5 @@ > + public void onConfigurationChanged(Configuration newConfig) { > + super.onConfigurationChanged(newConfig); > + > + // Rotation should detach and re-attach to use a different layout. > + if (isVisible()) { This piece is doubtful. If I open the bookmarks page. And rotate the device. Now will the history page re-inflate itself for new configuration? ::: mobile/android/base/resources/layout-large-land-v11/home_history_page.xml @@ +8,5 @@ > + android:layout_width="fill_parent" > + android:layout_height="fill_parent"> > + > + <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget" > + style="@style/Widget.Home.IconTabWidget" Ideally, this is not needed. You could add a global theme attribute, "iconTabWidgetStyle" and specify it at theme level. That's a cleaner approach as this is a common style for all "IconTabWidget"s. ::: mobile/android/base/resources/layout-large-land-v11/home_history_tabs_indicator.xml @@ +3,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<TextView xmlns:android="http://schemas.android.com/apk/res/android" > + style="@style/Widget.Home.PageTitle" No spaces here please. ::: mobile/android/base/resources/layout-xlarge-v11/home_history_page.xml @@ +8,5 @@ > + android:layout_width="fill_parent" > + android:layout_height="fill_parent"> > + > + <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget" > + style="@style/Widget.Home.IconTabWidget" Where is the information about the "layout" in the layout file? This is confusing. ::: mobile/android/base/resources/layout-xlarge-v11/home_history_tabs_indicator.xml @@ +3,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<TextView xmlns:android="http://schemas.android.com/apk/res/android" > + style="@style/Widget.Home.PageTitle" No spaces. ::: mobile/android/base/resources/layout/home_history_tabs_indicator.xml @@ +8,2 @@ > android:layout_height="fill_parent" > + android:layout_weight="1" Nice :) ::: mobile/android/base/resources/layout/home_list_with_title.xml @@ +28,5 @@ > > </LinearLayout> > > <TextView android:id="@+id/title" > + style="@style/Widget.Home.TabsPageTitle"/> Why is this name change here? If you want to use this name here, Home.PageTitle doesn't convey its a style for indicators, above. @@ +33,4 @@ > > <org.mozilla.gecko.home.HomeListView > android:id="@+id/list" > + style="@style/Widget.Home.ListView"/> No please noooooooooooo. I need layout information here. ::: mobile/android/base/resources/values-large-land-v11/styles.xml @@ +35,5 @@ > <item name="android:verticalSpacing">20dp</item> > </style> > > + <style name="Widget.Home.TabsPageTitle" parent="Widget.Home.PageTitle"> > + <item name="android:visibility">gone</item> Magic :( @@ +41,5 @@ > + > + <style name="Widget.Home.ListView"> > + <item name="android:layout_width">fill_parent</item> > + <item name="android:layout_height">0dp</item> > + <item name="android:layout_weight">1</item> Noooooooooo. Layout information should be in layout file. This is all magiccccc... :( ::: mobile/android/base/resources/values-xlarge-land-v11/styles.xml @@ +13,5 @@ > <item name="android:horizontalSpacing">56dp</item> > <item name="android:verticalSpacing">20dp</item> > </style> > > + <style name="Widget.Home.ListView"> Do you need these styles again here?? ::: mobile/android/base/resources/values-xlarge-v11/styles.xml @@ +11,3 @@ > --> > > <!-- TabWidget --> That's ok. You can remove this trailing space. ;) @@ +23,5 @@ > + <style name="Widget.Home.ListView"> > + <item name="android:layout_width">fill_parent</item> > + <item name="android:layout_height">0dp</item> > + <item name="android:layout_weight">1</item> > + <item name="android:paddingTop">30dp</item> There should be a padding right of 32dp on portrait mode. That's how other pages are. @@ +31,5 @@ > + <style name="Widget.Home.IconTabWidget"> > + <item name="android:layout_width">270dp</item> > + <item name="android:layout_height">wrap_content</item> > + <item name="android:orientation">vertical</item> > + <item name="android:paddingTop">30dp</item> There should be a padding left of 32dp on portrait mode. That's how other pages are. ::: mobile/android/base/resources/values/attrs.xml @@ +198,5 @@ > > <declare-styleable name="IconTabWidget"> > <attr name="android:layout"/> > + > + <!-- Sets the tab's content type. Defaults to icon. --> Nice nice on the "defaults" :) @@ +201,5 @@ > + > + <!-- Sets the tab's content type. Defaults to icon. --> > + <attr name="display"> > + <enum name="icon" value="0x00" /> > + <enum name="text" value="0x01" /> Nice :) ::: mobile/android/base/resources/values/styles.xml @@ +197,5 @@ > <item name="android:paddingRight">10dip</item> > </style> > > + <style name="Widget.Home.TabsPageTitle" parent="Widget.Home.PageTitle"> > + <item name="android:visibility">visible</item> Magic :( @@ +203,5 @@ > + > + <style name="Widget.Home.ListView"> > + <item name="android:layout_width">fill_parent</item> > + <item name="android:layout_height">0dp</item> > + <item name="android:layout_weight">1</item> Nooooo.. :(
Attachment #787327 - Flags: feedback?(sriram) → feedback+
Hi Shilpan, some quick visual feedback on some screenshots I saw. Landscape: http://cl.ly/image/3p3C1925220a Portrait: http://cl.ly/image/0I0u3E141T3H
Blocks: 903532
New patch with suggested changes.
Attachment #785272 - Attachment is obsolete: true
Attachment #785282 - Attachment is obsolete: true
Attachment #787327 - Attachment is obsolete: true
Attachment #788387 - Flags: review?(sriram)
Attached image 7" landscape —
Attached image 7" portrait —
Attached image 10" landscape —
The space is below the selected tab is only on 4.0
Attached image 10" portrait —
Comment on attachment 788387 [details] [diff] [review] Patch: Layout for tablet, history page Review of attachment 788387 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty solid. 95% closer to getting landed. I am a bit OCD about names used, as the names should convey what it is and help maintain the code. Hence asking you for another iteration to get the names right. In turn, I'll push the patch for you today! :D ::: mobile/android/base/Makefile.in @@ +530,5 @@ > $(NULL) > > RES_LAYOUT_XLARGE_V11 = \ > + res/layout-xlarge-v11/home_history_page.xml \ > + res/layout-xlarge-v11/home_history_tabs_indicator.xml \ We order these in alphabetical order. So move these below font.. ;) @@ +572,5 @@ > $(NULL) > > RES_VALUES_LARGE_LAND_V11 = \ > res/values-large-land-v11/styles.xml \ > + res/values-large-land-v11/dimens.xml \ Move it one line above. ABCD.. ;) @@ +583,5 @@ > $(NULL) > > RES_VALUES_XLARGE_LAND_V11 = \ > res/values-xlarge-land-v11/styles.xml \ > + res/values-xlarge-land-v11/dimens.xml \ Move it one line above. ABCD.. ;) @@ +1000,5 @@ > > +RES_DRAWABLE_LARGE_LAND_V11 = \ > + res/drawable-large-land-v11/home_history_tabs_indicator.xml \ > + > +RES_DRAWABLE_XLARGE_V11 = \ Group this with DRAWABLE_XLARGE ones. It's in-between two LARGE. ::: mobile/android/base/resources/layout-large-land-v11/home_history_page.xml @@ +8,5 @@ > + android:layout_width="fill_parent" > + android:layout_height="fill_parent"> > + > + <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget" > + style="@style/Widget.Home.IconTabWidget" Rename the style to Widget.HistoryTabWidget. That makes things clear. @@ +9,5 @@ > + android:layout_height="fill_parent"> > + > + <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget" > + style="@style/Widget.Home.IconTabWidget" > + android:layout_width="@dimen/home_tab_widget_width" history_tab_widget_width @@ +10,5 @@ > + > + <org.mozilla.gecko.widget.IconTabWidget android:id="@+id/tab_icon_widget" > + style="@style/Widget.Home.IconTabWidget" > + android:layout_width="@dimen/home_tab_widget_width" > + android:layout_height="@dimen/home_tab_widget_height" history_tab_widget_height ::: mobile/android/base/resources/layout-large-land-v11/home_history_tabs_indicator.xml @@ +3,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<TextView xmlns:android="http://schemas.android.com/apk/res/android" > + style="@style/Widget.Home.PageTitle" Rename this to Widget.HistoryTabIndicator. @@ +5,5 @@ > + > +<TextView xmlns:android="http://schemas.android.com/apk/res/android" > + style="@style/Widget.Home.PageTitle" > + android:layout_width="fill_parent" > + android:layout_height="@dimen/validation_message_height" What is validation_message? Please create a new dimen "history_tab_indicator_height" and use it. ::: mobile/android/base/resources/layout/home_list_with_title.xml @@ +28,5 @@ > > </LinearLayout> > > <TextView android:id="@+id/title" > + style="@style/Widget.Home.TabsPageTitle" Leave this as Widget.Home.PageTitle. Or, rename this to Widget.HistoryPageTitle. @@ +33,5 @@ > android:visibility="gone"/> > > <org.mozilla.gecko.home.HomeListView > android:id="@+id/list" > + style="@style/Widget.Home.ListView" This could be renamed to Widget.HistoryListView. There is a Widget.HomeListView, and I don't want the names sound similar. This XML is used in history pages only and hence be called as Widget.HistoryListView. ::: mobile/android/base/resources/values-large-land-v11/dimens.xml @@ +4,5 @@ > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<resources> > + > + <dimen name="home_tab_widget_width">360dp</dimen> Rename to "history_tab_widget_width". Only history page has it. It would be more appropriate to name it "home_" if its common for many pages / entire home. ::: mobile/android/base/resources/values/dimens.xml @@ +78,5 @@ > > + <!-- We need to maintain height for the tab widget on History Page > + since android does not add footer/header divider height to its > + calculation for wrap_content in LinearLayout.--> > + <dimen name="home_tab_widget_height">184dp</dimen> The calculation is bit wrong i guess. 50dp * 3 + 4dp right? Please make changes if needed and add a comment on how you arrive at this value. ::: mobile/android/base/resources/values/styles.xml @@ +197,5 @@ > <item name="android:paddingRight">10dip</item> > </style> > > + <style name="Widget.Home.TabsPageTitle" parent="Widget.Home.PageTitle"> > + <item name="android:visibility">visible</item> I am seeing some collision here. This is made visible. However the XML says gone. And android will take the XML value. So, this style is redundant. Please remove this. @@ +201,5 @@ > + <item name="android:visibility">visible</item> > + </style> > + > + <style name="Widget.Home.ListView"> > + <item name="android:scrollbarStyle">outsideOverlay</item> You might not need this. This is for phones right?
Attachment #788387 - Flags: review?(sriram) → review-
Made the requested changes. Also fixed the title bug. Should work on all devices.
Attachment #788387 - Attachment is obsolete: true
Attachment #788464 - Flags: review?(sriram)
Comment on attachment 788464 [details] [diff] [review] Patch: Layout for tablet, history page Review of attachment 788464 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r+ with changes mentioned. Please clean those up and attach a new patch to be uploaded. Please file followups for other recommended sections (like having a title in GONE state in tablets). ::: mobile/android/base/Makefile.in @@ +1000,5 @@ > res/drawable-xhdpi-v11/ic_status_logo.png \ > $(NULL) > > +RES_DRAWABLE_LARGE_LAND_V11 = \ > + res/drawable-large-land-v11/home_history_tabs_indicator.xml \ Add a $(NULL) at the end. @@ +1025,5 @@ > $(NULL) > > +RES_DRAWABLE_XLARGE_V11 = \ > + res/drawable-xlarge-v11/home_history_tabs_indicator.xml \ > + Add a $(NULL) at the end. ::: mobile/android/base/home/MostRecentPage.java @@ +158,5 @@ > return; > } > > // Cursor is empty, so hide the title and set the empty view if it hasn't been set already. > + if (mTitle != null) { I'm hating all these null checks. May be we should take a different approach -- may be in a followup. ::: mobile/android/base/resources/layout-large-land-v11/home_history_tabs_indicator.xml @@ +3,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<TextView xmlns:android="http://schemas.android.com/apk/res/android" > + style="@style/Widget.Home.PageTitle" I think I wanted this to be changed to HistoryTabIndicator or something. ::: mobile/android/base/resources/layout-large-land-v11/home_list_with_title.xml @@ +2,5 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<merge xmlns:android="http://schemas.android.com/apk/res/android"> This file is called home_list_with_title. But where's the title?? Better rename it to home_history_list in that case. This is confusing. If you have the TextView for title with the state as GONE, your code wouldn't have to do all those null checks. May be do this in a followup. ::: mobile/android/base/resources/layout-xlarge-v11/home_history_tabs_indicator.xml @@ +6,5 @@ > +<TextView xmlns:android="http://schemas.android.com/apk/res/android" > + style="@style/Widget.Home.PageTitle" > + android:layout_width="fill_parent" > + android:layout_height="@dimen/history_tab_indicator_height" > + android:paddingBottom="@dimen/toast_button_padding" Is this value going to vary depending on orientation? If not, I would recommend having the actual value here. ::: mobile/android/base/resources/layout-xlarge-v11/home_list_with_title.xml @@ +25,5 @@ > + android:layout_height="wrap_content" > + android:gravity="center" > + android:textAppearance="@style/TextAppearance.EmptyMessage"/> > + > + </LinearLayout> This is all moved into a ViewStub now. You might need to clean this up with a followup. ::: mobile/android/base/resources/layout/home_list_with_title.xml.rej @@ +1,2 @@ > +--- home_list_with_title.xml > ++++ home_list_with_title.xml Aaah! this is a rej file. Remove this!
Attachment #788464 - Flags: review?(sriram) → review+
Carry forward r+, made changes as mentioned above.
Attachment #788464 - Attachment is obsolete: true
Attachment #789243 - Flags: review+
"added 1 changesets with 29 changes to 29 files" http://hg.mozilla.org/projects/fig/rev/673e9806d6c3
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
Depends on: 906822
https://hg.mozilla.org/mozilla-central/rev/673e9806d6c3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: