1397888 - Top sites title: move pin icon to top left

Status: VERIFIED FIXED

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

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: 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.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Pin assets (already png optimized)

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Bug 1397888: Remove pin icon from top sites title.

Review commit: https://reviewboard.mozilla.org/r/179560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179560/

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Bug 1397888: Add top site pin assets.

Review commit: https://reviewboard.mozilla.org/r/179562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179562/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Bug 1397888: Display pin icon in top-left of top sites card if pinned.

Review commit: https://reviewboard.mozilla.org/r/179564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179564/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Screenshot post-patch: N5

Comment 6

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8907879 [details]
Bug 1397888: Display pin icon in top-left of top sites card if pinned.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179564/diff/1-2/

Comment 7

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Screenshot post-patch: NS

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

N7 also looks good.

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8907877 [details]
Bug 1397888: Remove pin icon from top sites title.

https://reviewboard.mozilla.org/r/179560/#review185102

Comment 10

[Chenxia Liu [:liuche]]

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.

Comment 11

[Chenxia Liu [:liuche]]

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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 13

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Bug 1397888 - review: Rename topsite_pin -> as_pin_with_background.

Review commit: https://reviewboard.mozilla.org/r/179938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179938/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8907877 [details]
Bug 1397888: Remove pin icon from top sites title.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179560/diff/1-2/

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8907878 [details]
Bug 1397888: Add top site pin assets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179562/diff/1-2/

Comment 16

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8907879 [details]
Bug 1397888: Display pin icon in top-left of top sites card if pinned.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179564/diff/2-3/

Comment 17

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8908306 [details]
Bug 1397888 - review: Rename topsite_pin -> as_pin_with_background.

https://reviewboard.mozilla.org/r/179938/#review185150

r=trivial

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Bogdan Surd, QA [:bsurd, NI]]

Verified in 57.0b3. A icon is properly displayed in the top left corner.