1383735 - Activity Stream: Update highlights layout to match latest mocks

Status: RESOLVED FIXED

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

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sebastian, it looks like bug 1379021 (which seems to have addressed the highlights layout) was closed but bug 1380639 (ConstraintLayout to the build system) was not - what changes still need to be addressed for this bug?

We're trying to figure out what's been unblocked from the P2 list with respect highlights.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

I created those bugs (highlights / top sites) as follow-up bugs because I couldn't land all changes without ConstraintLayout.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

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

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

https://reviewboard.mozilla.org/r/164552/#review170106

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

I can't land this anymore. ConstraintLayout is making issues - see bug 1388023. I had the sheriffs remove it from central again: bug 1380639.

I'll rewrite this to use regular layouts and then we can migrate later.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

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

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

@mcomella: Can you have a look at the updated patch (I'm not sure how to re-request it from mozreview). This is a version that does not use ConstraintLayout anymore. Luckily I was able to simplify the layout nevertheless.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

https://reviewboard.mozilla.org/r/164552/#review170922

::: mobile/android/app/src/main/res/layout/activity_stream_card_history_item.xml:20
(Diff revision 2)
> -        <FrameLayout
> +    <FrameLayout
> -            android:id="@+id/icon_wrapper"
> +        android:id="@+id/icon_wrapper"
> -            android:layout_width="wrap_content"
> +        android:layout_width="wrap_content"
> -            android:layout_height="wrap_content">
> +        android:layout_height="wrap_content">
>  
> -            <org.mozilla.gecko.widget.FaviconView
> +        <org.mozilla.gecko.widget.FaviconView

This probably doesn't need a contentDescription, right? This discussion could also go into one of the a11y bugs.

::: mobile/android/app/src/main/res/layout/activity_stream_card_history_item.xml:41
(Diff revision 2)
> -            android:layout_margin="2dp"
> -            android:layout_alignParentEnd="true"
> +        android:layout_alignParentEnd="true"
> -            android:layout_alignParentRight="true"
> +        android:layout_alignParentRight="true"
> -            android:layout_alignParentTop="true"
> +        android:layout_alignParentTop="true"
> -            android:layout_gravity="right|end|top"
> +        android:layout_gravity="right|end|top"
> +        android:layout_margin="0dp"

nit: Is margin = 0 necessary? I thought it was the default value.

::: mobile/android/app/src/main/res/layout/activity_stream_card_history_item.xml:45
(Diff revision 2)
> -            android:layout_gravity="right|end|top"
> +        android:layout_gravity="right|end|top"
> +        android:layout_margin="0dp"
> -            android:contentDescription="@string/menu"
> +        android:contentDescription="@string/menu"
> -            android:src="@drawable/menu"
> -            android:padding="@dimen/activity_stream_base_margin" />
> +        android:paddingBottom="16dp"
> +        android:paddingLeft="0dp"
> +        android:paddingRight="0dp"

nit: same – is specifying 0 necessary? Or perhaps this is for clarity?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

https://reviewboard.mozilla.org/r/164552/#review170922

> This probably doesn't need a contentDescription, right? This discussion could also go into one of the a11y bugs.

Yeah, I think the favicon is only a indicator that helps you (visually) navigate.

> nit: Is margin = 0 necessary? I thought it was the default value.

You are right. This is an artifact from iterating on the design.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/450405f6daaa
Activity Stream: Update highlights layout to match latest mocks. r=mcomella

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/450405f6daaa