Closed
Bug 1397888
Opened 6 years ago
Closed 5 years ago
Top sites title: move pin icon to top left
Categories
(Firefox for Android Graveyard :: Awesomescreen, enhancement, P1)
Tracking
(firefox57 verified)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: mcomella, Assigned: mcomella)
Details
(Whiteboard: [mobileAS])
Attachments
(8 files)
72.63 KB,
image/png
|
Details | |
1.47 KB,
application/x-gzip
|
Details | |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
134.50 KB,
image/png
|
Details | |
64.22 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
The pin in the current position takes characters away from the title when we pin an item. Since there already isn't a lot of space, this isn't great. bbell suggested we move the pin to the top left.
Assignee | ||
Updated•6 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Updated•6 years ago
|
Rank: 2
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
tracking-fennec: ? → ---
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #8907360 -
Attachment description: Pin assets → Pin assets (already png optimized)
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Updated•5 years ago
|
Iteration: --- → 1.30
Rank: 2
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•5 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
N7 also looks good.
Comment 9•5 years ago
|
||
mozreview-review |
Comment on attachment 8907877 [details] Bug 1397888: Remove pin icon from top sites title. https://reviewboard.mozilla.org/r/179560/#review185102
Attachment #8907877 -
Flags: review?(liuche) → review+
Comment 10•5 years ago
|
||
mozreview-review |
Comment on attachment 8907878 [details] Bug 1397888: Add top site pin assets. https://reviewboard.mozilla.org/r/179562/#review185104 Might cause a lint failure, see comment and double-check the resource you're updating. ::: commit-message-168e9:1 (Diff revision 1) > +Bug 1397888: Add top site pin assets. r=liuche Is this a rename of the old top sites pin? The pin you removed in the previous patch was called as_pin, so if you need to either make changes to the as resource and keep using it, or remove it so you don't get lint failures.
Attachment #8907878 -
Flags: review?(liuche) → review+
Comment 11•5 years ago
|
||
mozreview-review |
Comment on attachment 8907879 [details] Bug 1397888: Display pin icon in top-left of top sites card if pinned. https://reviewboard.mozilla.org/r/179564/#review185134 ::: mobile/android/app/src/main/res/layout/activity_stream_topsites_card.xml:23 (Diff revision 2) > + android:id="@+id/pin_icon" > + android:layout_width="wrap_content" > + android:layout_height="wrap_content" > + android:layout_gravity="top|start" > + android:layout_margin="3dp" > + android:src="@drawable/topsite_pin" Is this the right icon? I'm wondering what as_pin is, and why it's used.
Attachment #8907879 -
Flags: review?(liuche) → review+
Assignee | ||
Comment 12•5 years ago
|
||
mozreview-review-reply |
Comment on attachment 8907878 [details] Bug 1397888: Add top site pin assets. https://reviewboard.mozilla.org/r/179562/#review185104 > Is this a rename of the old top sites pin? The pin you removed in the previous patch was called as_pin, so if you need to either make changes to the as resource and keep using it, or remove it so you don't get lint failures. as_pin is used in the context menu: http://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+as_pin&path= I'll rename them so it's clearer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•5 years ago
|
||
mozreview-review |
Comment on attachment 8908306 [details] Bug 1397888 - review: Rename topsite_pin -> as_pin_with_background. https://reviewboard.mozilla.org/r/179938/#review185150 r=trivial
Attachment #8908306 -
Flags: review?(michael.l.comella) → review+
Comment 18•5 years ago
|
||
Pushed by michael.l.comella@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5289d258b565 Remove pin icon from top sites title. r=liuche https://hg.mozilla.org/integration/autoland/rev/adab05485ec5 Add top site pin assets. r=liuche https://hg.mozilla.org/integration/autoland/rev/e94744cad7d8 Display pin icon in top-left of top sites card if pinned. r=liuche https://hg.mozilla.org/integration/autoland/rev/736023670ba8 review: Rename topsite_pin -> as_pin_with_background. r=mcomella
![]() |
||
Comment 19•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5289d258b565 https://hg.mozilla.org/mozilla-central/rev/adab05485ec5 https://hg.mozilla.org/mozilla-central/rev/e94744cad7d8 https://hg.mozilla.org/mozilla-central/rev/736023670ba8
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 20•5 years ago
|
||
Verified in 57.0b3. A icon is properly displayed in the top left corner.
Status: RESOLVED → VERIFIED
Updated•2 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
•