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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(6 files, 12 obsolete files)
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
application/zip
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
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.
Updated•10 years ago
|
Attachment #8516811 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 4•10 years ago
|
||
What say you, antlam?
Attachment #8516817 -
Flags: feedback?(alam)
Comment 5•10 years ago
|
||
Comment on attachment 8516817 [details]
Centered
Looking good!
Attachment #8516817 -
Flags: feedback?(alam) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6c5dd1a28d60 https://hg.mozilla.org/integration/fx-team/rev/7db917954ac6
Assignee | ||
Comment 7•10 years ago
|
||
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]
Updated•10 years ago
|
Attachment #8516816 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Undoing part 1 in a new patch because part 1 already landed. Oops!
Attachment #8517387 -
Flags: review?(lucasr.at.mozilla)
Comment 10•10 years ago
|
||
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+
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6c5dd1a28d60 https://hg.mozilla.org/mozilla-central/rev/7db917954ac6
Assignee | ||
Comment 13•10 years ago
|
||
The new bookmarks icon looks a bit funky - any ideas, Anthony?
Attachment #8518070 -
Flags: feedback?(alam)
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Updated the unfilled icon but here's the full set anyways (filled ones haven't changed).
Attachment #8518035 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Summary: Investigate reload misalignment → Correct new tablet action bar alignment
Assignee | ||
Updated•10 years ago
|
Summary: Correct new tablet action bar alignment → Correct new tablet menu bar item alignment
Assignee | ||
Comment 18•10 years ago
|
||
irl antlam approved.
Attachment #8518156 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8518281 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8518071 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8517387 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8516817 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8517384 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
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.
Assignee | ||
Comment 21•10 years ago
|
||
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)
Updated•10 years ago
|
Flags: needinfo?(lucasr.at.mozilla)
Priority: -- → P1
Assignee | ||
Updated•10 years ago
|
Attachment #8518887 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8518878 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8518885 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Comment 25•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8524824 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8525719 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [leave-open]
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5089a4ce113b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•