I spoke with antlam and nalexander on IRC and we decided to land the functionality and clean up the visual aspects in this followup. One outlier, via bug 958889 comment 6: > It's looking good. I would focus on the alignment of the baseline of "Using > an older version of Sync?" and "...Bookmarks, passwords & more." Keep in mind how l10n will change this.

Some nits Anthony had in previous bugs: * Bug 1014999 comment 9: On tablets, the "Get Started" button should be the same width as the "Welcome to Sync" text - consider defining this button (and other buttons) in absolute sizes, rather than as padding * Bug 958889 comment 56 (same as comment 0): In the dual column layout of the Setup screen, "the copy that reads "Using an old version of Sync?" and "Sign in to sync your tabs bookmarks and more" should have their bottoms align." Additionally, try different languages, and correct for locale issues. Anthony, have you had a chance to take a look at this in Nightly? Is there anything else you might want to add? Note that the tablet version should first be in the 5/31 Nightly.

Also, different screen sizes!

Having padding set the button width made the button size dependent on the text size, so I went with match_parent and margins this time around.

I think this much paddingTop looks a bit silly on small devices, so I'm adjusting that in part 3.

Comment on attachment 8436211 [details] [diff] [review] Part 3: Reduce padding at the top of portrait remote tabs panel. Actually, Lucas is a little more in touch with these styles.

The aftermath.

Comment on attachment 8436206 [details] [diff] [review] Part 2: Make remote tabs panel buttons more consistent across devices. Forgot tablet love.

Anthony, what do you think of the decision to change the padding from the attachment in comment 7 to the attachments in comment 11 and comment 12?

Comment on attachment 8437251 [details] Xlarge setup button (portrait) I'm not too concerned about the landscape version.

Large device is the Kobo Arc, the Xlarge device is a Galaxy Tab 3. Note that the button is created by filling the screen with hard-coded margins, so it will not change sizes with l10n (with exception to line wrapping), but will change sizes with screen sizes. I'm not sure if this is the best practice - I'll get feedback from Lucas in the review.

Forgot I was supposed to merge part 4 with part 2. Lucas, is layout_width="match_parent" with hard-coded margins an okay approach to defining button sizes?

(In reply to Michael Comella (:mcomella) from comment #14) > Anthony, what do you think of the decision to change the padding from the > attachment in comment 7 to the attachments in comment 11 and comment 12? Yeah that works. (In reply to Michael Comella (:mcomella) from comment #17) > Comment on attachment 8437251 [details] > Xlarge setup button (portrait) > > I'm not too concerned about the landscape version. The text wrapping could be done better here. Can we get the line to wrap after "tabs,"? Also, we might want to try padding further from the tab since on this device it seems the active tab is a long way's down. (In reply to Michael Comella (:mcomella) from comment #18) > Created attachment 8437256 [details] > Large setup button (portrait) > > Large device is the Kobo Arc, the Xlarge device is a Galaxy Tab 3. > > Note that the button is created by filling the screen with hard-coded > margins, so it will not change sizes with l10n (with exception to line > wrapping), but will change sizes with screen sizes. I'm not sure if this is > the best practice - I'll get feedback from Lucas in the review. Can we get the line to wrap after "tabs," as well?

Lucas, is it reasonable to define the width of a view in absolute units (as opposed to match_parent with margin sizes)? Using dp, this would allow us to ensure the width of the text is relatively consistent (and thus readable, like a consistent page width) across devices. Since each layout has a minimum size of dp, we can use this information that the width is never cut off across devices. For context, read on. (In reply to Anthony Lam (:antlam) from comment #22) > The text wrapping could be done better here. Can we get the line to wrap > after "tabs,"? Currently, the width of this text is decided by margins, meaning this wrapping is particular to this device (and other similar size/density screens). If we change the margins, we change the size of the text across devices (and less relevantly, languages). Is this okay? Alternatively, I asked Lucas above whether or not we can define absolute widths for the text column, so we can wait until we see what he has to say.

(In reply to Michael Comella (:mcomella) from comment #23) > Lucas, is it reasonable to define the width of a view in absolute units (as > opposed to match_parent with margin sizes)? Using dp, this would allow us to > ensure the width of the text is relatively consistent (and thus readable, > like a consistent page width) across devices. Since each layout has a > minimum size of dp, we can use this information that the width is never cut > off across devices. For context, read on. > > (In reply to Anthony Lam (:antlam) from comment #22) > > The text wrapping could be done better here. Can we get the line to wrap > > after "tabs,"? > > Currently, the width of this text is decided by margins, meaning this > wrapping is particular to this device (and other similar size/density > screens). If we change the margins, we change the size of the text across > devices (and less relevantly, languages). Is this okay? > > Alternatively, I asked Lucas above whether or not we can define absolute > widths for the text column, so we can wait until we see what he has to say. I wouldn't go to far in terms of enforcing specific line wrapping behaviour here because this is almost guaranteed to look different in each locale. If we *really* want this, it would be better to just add a newline character to the string itself with a matching l10n comment explaining the UI guidelines around it. Please check with Pike if this is a valid l10n practice.

Comment on attachment 8437266 [details] [diff] [review] Part 2: Make remote tabs panel buttons more consistent across devices.* * * Review of attachment 8437266 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

(In reply to Lucas Rocha (:lucasr) from comment #24) > > (In reply to Anthony Lam (:antlam) from comment #22) > > > The text wrapping could be done better here. Can we get the line to wrap > > > after "tabs,"? > > > > Currently, the width of this text is decided by margins, meaning this > > wrapping is particular to this device (and other similar size/density > > screens). If we change the margins, we change the size of the text across > > devices (and less relevantly, languages). Is this okay? > > > > Alternatively, I asked Lucas above whether or not we can define absolute > > widths for the text column, so we can wait until we see what he has to say. > > I wouldn't go to far in terms of enforcing specific line wrapping behaviour > here because this is almost guaranteed to look different in each locale. If > we *really* want this, it would be better to just add a newline character to > the string itself with a matching l10n comment explaining the UI guidelines > around it. Please check with Pike if this is a valid l10n practice. I agree - that's just how I've been keeping tabs on cross-device spacial alignment in my head but I get that it doesn't make sense from an implementation point of view.

remote: https://hg.mozilla.org/integration/fx-team/rev/8ef009362d13 remote: https://hg.mozilla.org/integration/fx-team/rev/1e480afce4b3 remote: https://hg.mozilla.org/integration/fx-team/rev/83be53bc8db9 Anthony, this should land in tomorrow's Nightly (6/12) - do you want to give it a spin and ensure all there are no more changes to make?

https://hg.mozilla.org/mozilla-central/rev/8ef009362d13 https://hg.mozilla.org/mozilla-central/rev/1e480afce4b3 https://hg.mozilla.org/mozilla-central/rev/83be53bc8db9

Comment on attachment 8437251 [details] Xlarge setup button (portrait) Spacing along the bottom is a bit concerning but I think it should be considered as a part of a bigger issue.

Marking FIXED as there are no more feedback requests for antlam.

Comment on attachment 8436191 [details] [diff] [review] Part 1: Add scrolling to remote tabs tray where applicable. This request applies to parts 1-3. [Approval Request Comment] Bug caused by (feature/regressing bug #): Adding the setup and verification panels to the remote tabs panel (see dependent bugs) User impact if declined: Users will see an unpolished version of the remote tabs panel UI. Additionally, users of small screen devices or long languages will be unable to see the full content (a scroll view was added). Testing completed (on m-c, etc.): Local Risk to taking this patch (and alternatives if risky): Low - these are mostly formatting changes. However, these patches *do* introduce a bug which is solved by the attachment in bug 1024778 comment 1, so that should be uplifted too. String or IDL/UUID changes made by this patch: None

Verified as fixed in Build: 33.0a1 (2014-06-16) Devices: Samsung S3 (Android 4.3) Samsung Galaxy R (Android 2.3.4) Motorola Razr (Android 4.0.4) Asus Transformer Pad TF300T (Android 4.2.1) AGPTEK TP10A (Android 4.0.4)

I'm unclear from the uplift request whether this change ended up having l10n impact such as that described in comment 18 and comment 24. Does the l10n team need to know about this change?

(In reply to Lawrence Mandel [:lmandel] from comment #33) > I'm unclear from the uplift request whether this change ended up having l10n > impact such as that described in comment 18 and comment 24. Does the l10n > team need to know about this change? No, in comment 18 we were discussing how the View layouts might change with longer or shorter text. Ideally, this would be little to none, so the l10n team will likely have nothing to add. In comment 24, we were discussing using newlines to force word wrapping but decided this didn't make sense given variable screen sizes, didn't make the change, and thus do not need l10n input.

Comment on attachment 8436191 [details] [diff] [review] Part 1: Add scrolling to remote tabs tray where applicable. Thanks for the confirmation about no l10n impact. Aurora approval granted.

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/44ef5ff8a390 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/3b319d395f50 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/71f5fa1512d2

Comment on attachment 8436211 [details] [diff] [review] Part 3: Reduce padding at the top of portrait remote tabs panel. Missed setting approval earlier.

Comment on attachment 8437266 [details] [diff] [review] Part 2: Make remote tabs panel buttons more consistent across devices.* * * Missed setting approval earlier.