Closed Bug 1085487 Opened 10 years ago Closed 10 years ago

Correct new tablet menu bar item alignment

Categories

(Firefox for Android Graveyard :: General, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(6 files, 12 obsolete files)

3.85 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.81 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
12.05 KB, application/zip
Details
139.27 KB, image/png
Details
301.22 KB, image/png
Details
25.24 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
After the patch in bug 1079183, the alignment of the reload button looks a little off - the bottoms of the icons are horizontally aligned, but the reload button's top appears to be lower than the other icons.
Note also the tabs panel tab # text is not centered.
Attachment #8516811 - Flags: review?(lucasr.at.mozilla) → review+
Attached image Centered (obsolete) —
What say you, antlam?
Attachment #8516817 - Flags: feedback?(alam)
Comment on attachment 8516817 [details]
Centered

Looking good!
Attachment #8516817 - Flags: feedback?(alam) → feedback+
My apologies! I thought part 2 was reviewed. Also, we discovered on xlarge devices, the bookmarks button is also present, so we should probably *gasp* add white-space to the reload icon to compensate (rather than changing the alignment in code, as we did in part 1).
Whiteboard: [leave-open]
Attachment #8516816 - Flags: review?(lucasr.at.mozilla) → review+
Attached image xlarge tablet w/ bookmark star (obsolete) —
Anthony, Lucas and I noticed that the bookmarks button appears on 10" tablet but we don't have a new tablet resource for it (i.e. in mdpi, the bookmarks asset is 24x24 px, rather than the expected 18 px height) - can we get some appropriately sized assets?

In the tree, if you need it: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xlarge-mdpi-v11/
Flags: needinfo?(alam)
Undoing part 1 in a new patch because part 1 already landed. Oops!
Attachment #8517387 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8517387 [details] [diff] [review]
Part 3: Offset reload with whitespace in drawable; undo part 1

Review of attachment 8517387 [details] [diff] [review]:
-----------------------------------------------------------------

Yep.
Attachment #8517387 - Flags: review?(lucasr.at.mozilla) → review+
Attached file icon_bookmark.zip (obsolete) —
Here you go, Mike!
Flags: needinfo?(alam)
Attached image new bookmarks icon (obsolete) —
The new bookmarks icon looks a bit funky - any ideas, Anthony?
Attachment #8518070 - Flags: feedback?(alam)
Attached file icon_bookmark2.zip (obsolete) —
Updated the unfilled icon but here's the full set anyways (filled ones haven't changed).
Attachment #8518035 - Attachment is obsolete: true
Attached image new bookmarks icon (obsolete) —
As discussed IRL, this asset probably needs to be bigger - NI antlam to get that.

Note to self: may need to dynamically change sizes based on ID then (x_x).
Attachment #8518070 - Attachment is obsolete: true
Attachment #8518070 - Flags: feedback?(alam)
Flags: needinfo?(alam)
Summary: Investigate reload misalignment → Correct new tablet action bar alignment
Summary: Correct new tablet action bar alignment → Correct new tablet menu bar item alignment
Attached file icon_bookmark3.zip
Pass 3!
Flags: needinfo?(alam)
irl antlam approved.
Attachment #8518156 - Attachment is obsolete: true
Positioned the action bar around the bookmarks button and added whitespace to
the reload button to compensate.

However, this patch is not finished because my xlarge tablet died (Transformer,
you will not be forgotten!). The initial state is not set up properly, nor is
the drawable update scheme - this was just a hack to get the images showing for
alignment purposes.
Lucas, unless fate shines brightly upon me, I won't have an xlarge tablet until 11/17 - if you want this to land sooner, feel free to finish this up.
Flags: needinfo?(lucasr.at.mozilla)
Flags: needinfo?(lucasr.at.mozilla)
Priority: -- → P1
Comment on attachment 8524824 [details] [diff] [review]
Part 3: Center new bookmarks button on xlarge tablet, compensate for reload button

Review of attachment 8524824 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I can't wait to see all these hacks removed :-)

::: mobile/android/base/BrowserApp.java
@@ +2730,5 @@
> +            removeResID = R.drawable.new_tablet_ic_menu_bookmark_remove;
> +        } else {
> +            addResID = R.drawable.ic_menu_bookmark_add;
> +            removeResID = R.drawable.ic_menu_bookmark_remove;
> +        }

Factor out this resource ID code into a separate method like resolveBookmarkIconID(boolean isBookmark) to avoid code duplication.

@@ +2857,5 @@
>                      Telemetry.sendUIEvent(TelemetryContract.Event.UNSAVE, TelemetryContract.Method.MENU, "bookmark");
>                      tab.removeBookmark();
> +                    final int drawableID = (NewTabletUI.isEnabled(this) && HardwareUtils.isLargeTablet()) ?
> +                            R.drawable.new_tablet_ic_menu_bookmark_add : R.drawable.ic_menu_bookmark_add;
> +                    item.setIcon(drawableID);

This should become item.setIcon(resolveBookmarkIconID(false)) if you follow my suggestion above.

@@ +2863,5 @@
>                      Telemetry.sendUIEvent(TelemetryContract.Event.SAVE, TelemetryContract.Method.MENU, "bookmark");
>                      tab.addBookmark();
> +                    final int drawableID = (NewTabletUI.isEnabled(this) && HardwareUtils.isLargeTablet()) ?
> +                            R.drawable.new_tablet_ic_menu_bookmark_remove : R.drawable.ic_menu_bookmark_remove;
> +                    item.setIcon(drawableID);

Ditto.
Attachment #8524824 - Flags: review?(lucasr.at.mozilla) → review+
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/5089a4ce113b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.