I realized you didn't get to review all of the UI changes in the previous bug - perhaps you want to audit all of the layout code here.

Anthony, how's this? If this is acceptable to ship, I'd like to do fine tuning in bug 1007442 because I'll also have to figure out all the l10n issues.

I assume these next two are okay so I'm not flagging for feedback.

Comment on attachment 8430340 [details] [diff] [review] Add remote tabs setup/verification panels for tablet. Review of attachment 8430340 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please audit your patch for any redundant style definitions. ::: mobile/android/base/resources/values-large-land-v11/styles.xml @@ +59,5 @@ > + <item name="android:paddingRight">0dp</item> > + </style> > + > + <style name="RemoteTabsSection" parent="RemoteTabsSectionBase"> > + <!-- To override the values-land style. --> You don't set any values in values-land's RemoteTabsSection either. This seems a bit redundant, remove it. @@ +63,5 @@ > + <!-- To override the values-land style. --> > + </style> > + > + <style name="RemoteTabsItem" parent="RemoteTabsItemBase"> > + <!-- To override the values-land style. --> Ditto, this seems redundant as you're setting attribute in values-land's RemoteTabsItem either. Remove it.

Comment on attachment 8430346 [details] Setup horizontal Could we shrink the font size on the get started button down to the same as the body text here?

Comment on attachment 8431753 [details] Setup horizontal Looks better, almost there! :) Can we adjust the width of the CTA button here to be the width of the title "Welcome to Sync"? A nice rounded number will work.

(In reply to Lucas Rocha (:lucasr) from comment #6) > You don't set any values in values-land's RemoteTabsSection either. This > seems a bit redundant, remove it. RemoteTabsSection has no definitions in values/, has defs in values-land/ (layout_weight), and has no defs in values-large-land-v11/. We want to inherit some attr and override others in landscape, hence the use of RemoteTabsSectionBase and the emptiness of values/. However, on tablet, without values-large-land-v11/, values-land/ will be read if we have the device in landscape, when we actually want values/ (or more accurately, the base style). Thus I defined an empty style in values-large-land-v11/. Is there a better paradigm for this? > Ditto, this seems redundant as you're setting attribute in values-land's > RemoteTabsItem either. Remove it. See above.

Comment on attachment 8431767 [details] Setup horizontal Looking good

Updated w/ UI nits.

Comment on attachment 8430340 [details] [diff] [review] Add remote tabs setup/verification panels for tablet. ># HG changeset patch ># User Michael Comella <michael.l.comella@gmail.com> ># Date 1401312719 25200 ># Wed May 28 14:31:59 2014 -0700 ># Node ID b90d60ee8249065d10f2c401469b0ee32dc5ab47 ># Parent bc643e9ee8422e3a52258b613fedf1b36d42b023 >Bug 1014999 - Add remote tabs setup/verification panels for tablet. > >diff --git a/mobile/android/base/resources/values-large-land-v11/styles.xml b/mobile/android/base/resources/values-large-land-v11/styles.xml >--- a/mobile/android/base/resources/values-large-land-v11/styles.xml >+++ b/mobile/android/base/resources/values-large-land-v11/styles.xml >@@ -46,9 +46,40 @@ > > <style name="Widget.Home.HistoryTabWidget"> > <item name="android:showDividers">beginning|middle|end</item> > <item name="android:dividerPadding">0dp</item> > <item name="android:paddingLeft">100dp</item> > <item name="android:paddingTop">30dp</item> > </style> > >+ <!-- Remote tabs panel --> >+ <style name="RemoteTabsPanelChild" parent="RemoteTabsPanelChildBase"> >+ <item name="android:paddingTop">32dp</item> >+ <!-- Additional spacing set via margins on RemoteTabsSection. --> >+ <item name="android:paddingLeft">0dp</item> >+ <item name="android:paddingRight">0dp</item> >+ </style> >+ >+ <style name="RemoteTabsSection" parent="RemoteTabsSectionBase"> >+ <!-- To override the values-land style. --> >+ </style> >+ >+ <style name="RemoteTabsItem" parent="RemoteTabsItemBase"> >+ <!-- To override the values-land style. --> >+ </style> >+ >+ <style name="RemoteTabsItem.Button" parent="RemoteTabsItem.ButtonBase"> >+ <item name="android:paddingTop">12dp</item> >+ <item name="android:paddingBottom">12dp</item> >+ <item name="android:paddingLeft">24dp</item> >+ <item name="android:paddingRight">24dp</item> >+ </style> >+ >+ <style name="RemoteTabsItem.TextAppearance.Header.FXAccounts"> >+ <item name="android:visibility">gone</item> >+ </style> >+ >+ <style name="RemoteTabsItem.TextAppearance.Linkified.Resend"> >+ <item name="android:layout_height">wrap_content</item> >+ </style> >+ > </resources> >diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml >--- a/mobile/android/base/resources/values/styles.xml >+++ b/mobile/android/base/resources/values/styles.xml >@@ -447,43 +447,44 @@ > <item name="android:childDivider">@drawable/remote_tabs_child_divider</item> > <item name="android:groupIndicator">@android:color/transparent</item> > </style> > > <!-- Remote tabs panel --> > <style name="RemoteTabsPanelChildBase"> > <item name="android:paddingLeft">16dp</item> > <item name="android:paddingRight">16dp</item> >+ <item name="android:paddingTop">62dp</item> >+ <item name="android:orientation">vertical</item> > </style> > > <style name="RemoteTabsPanelChild" parent="RemoteTabsPanelChildBase"> >- <item name="android:orientation">vertical</item> >- <item name="android:paddingTop">62dp</item> >+ <!-- We set values in landscape. --> > </style> > > <style name="RemoteTabsSectionBase"> > <item name="android:orientation">vertical</item> > <item name="android:layout_marginLeft">16dp</item> > <item name="android:layout_marginRight">16dp</item> > </style> > > <style name="RemoteTabsSection" parent="RemoteTabsSectionBase"> > <!-- We set values in landscape. --> > </style> > >- <style name="RemoteTabsSection.Resend" parent="RemoteTabsSectionBase"> >- <!-- We set values in landscape. --> >- </style> >- >- <style name="RemoteTabsItem"> >+ <style name="RemoteTabsItemBase"> > <item name="android:layout_marginBottom">28dp</item> > <item name="android:layout_gravity">center</item> > <item name="android:gravity">center</item> > </style> > >+ <style name="RemoteTabsItem" parent="RemoteTabsItemBase"> >+ <!-- We set values in landscape. --> >+ </style> >+ > <style name="RemoteTabsItem.ButtonBase"> > <item name="android:background">@drawable/remote_tabs_setup_button_background</item> > <item name="android:textColor">#FFFEFF</item> > <item name="android:textSize">20sp</item> > </style> > > <style name="RemoteTabsItem.Button" parent="RemoteTabsItem.ButtonBase"> > <item name="android:paddingTop">18dp</item> >diff --git a/mobile/android/base/tabspanel/RemoteTabsSetupPanel.java b/mobile/android/base/tabspanel/RemoteTabsSetupPanel.java >--- a/mobile/android/base/tabspanel/RemoteTabsSetupPanel.java >+++ b/mobile/android/base/tabspanel/RemoteTabsSetupPanel.java >@@ -6,17 +6,16 @@ package org.mozilla.gecko.tabspanel; > > import java.util.Locale; > > import org.mozilla.gecko.R; > import org.mozilla.gecko.Tabs; > import org.mozilla.gecko.fxa.FirefoxAccounts; > import org.mozilla.gecko.fxa.activities.FxAccountCreateAccountActivity; > import org.mozilla.gecko.tabspanel.TabsPanel.PanelView; >-import org.mozilla.gecko.util.HardwareUtils; > > import android.content.Context; > import android.content.Intent; > import android.util.AttributeSet; > import android.view.View; > import android.widget.LinearLayout; > > /** >@@ -64,21 +63,16 @@ class RemoteTabsSetupPanel extends Linea > > @Override > public void setTabsPanel(TabsPanel panel) { > tabsPanel = panel; > } > > @Override > public void show() { >- // We don't have a tablet implementation of this panel. >- if (HardwareUtils.isTablet()) { >- return; >- } >- > setVisibility(View.VISIBLE); > } > > @Override > public void hide() { > setVisibility(View.GONE); > } > >diff --git a/mobile/android/base/tabspanel/RemoteTabsVerificationPanel.java b/mobile/android/base/tabspanel/RemoteTabsVerificationPanel.java >--- a/mobile/android/base/tabspanel/RemoteTabsVerificationPanel.java >+++ b/mobile/android/base/tabspanel/RemoteTabsVerificationPanel.java >@@ -3,17 +3,16 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > package org.mozilla.gecko.tabspanel; > > import org.mozilla.gecko.R; > import org.mozilla.gecko.fxa.FirefoxAccounts; > import org.mozilla.gecko.fxa.login.State; > import org.mozilla.gecko.tabspanel.TabsPanel.PanelView; >-import org.mozilla.gecko.util.HardwareUtils; > > import android.content.Context; > import android.util.AttributeSet; > import android.util.Log; > import android.view.View; > import android.widget.LinearLayout; > import android.widget.TextView; > >@@ -93,21 +92,16 @@ class RemoteTabsVerificationPanel extend > > @Override > public void setTabsPanel(TabsPanel panel) { > tabsPanel = panel; > } > > @Override > public void show() { >- // We don't have a tablet implementation of this panel. >- if (HardwareUtils.isTablet()) { >- return; >- } >- > refresh(); > setVisibility(View.VISIBLE); > } > > @Override > public void hide() { > setVisibility(View.GONE); > }

Verified as fixed in Build: Firefox for Android 33.0a2 (2014-08-19) Device: Asus Transformer Pad TF300T (Android 4.2.1)

(In reply to Cristina Madaras, QA [:CristinaM] from comment #17) > Verified as fixed in > Build: Firefox for Android 33.0a2 (2014-08-19) > Device: Asus Transformer Pad TF300T (Android 4.2.1) I meant Firefox for Android 32 Beta 8. Sorry for this!