Closed Bug 1397888 Opened 2 years ago Closed 2 years ago

Top sites title: move pin icon to top left

Categories

(Firefox for Android :: Awesomescreen, enhancement, P1)

Product:
Firefox for Android
Component:
Awesomescreen
Platform:
All
Android
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
1.30
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

Details

(Whiteboard: [mobileAS])

Attachments

(8 files)

Mocks
72.63 KB, image/png
Details
Pin assets (already png optimized)
1.47 KB, application/x-gzip
Details
Bug 1397888: Remove pin icon from top sites title.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1397888: Add top site pin assets.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1397888: Display pin icon in top-left of top sites card if pinned.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Screenshot post-patch: N5
134.50 KB, image/png
Details
Screenshot post-patch: NS
64.22 KB, image/png
Details
Bug 1397888 - review: Rename topsite_pin -> as_pin_with_background.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Attached image Mocks — 
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.
tracking-fennec: --- → ?
Rank: 2
Priority: -- → P2
tracking-fennec: ? → ---
Attachment #8907360 - Attachment description: Pin assets → Pin assets (already png optimized)
Assignee: nobody → michael.l.comella
Iteration: --- → 1.30
Rank: 2
Priority: P2 → P1
N7 also looks good.
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 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 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+
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 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+
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
Verified in 57.0b3. A icon is properly displayed in the top left corner.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.