1308525 - Activity Stream: Update measurements and highlights style

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Measurements

See attached image: The style of highlights changed a little bit. Additionally we should make sure to use the measurements defined in the spec.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1308525 - Update activity stream panel to follow new UX mocks.

Okay, this patch is much bigger than I anticipated. However at this point it is impossible
to separate all the interweaved changes.

* Update sizes and colors
* Use a dynamic number of top sites tiles to adjust to the screen size
* Do not keep references (to possible outdated) Cursors from every top sites page
* Remove unused bottom panel
* Add menu button to highlights items

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

Comment 3

[:Grisha Kruglov]

Comment on attachment 8800688 [details]
Bug 1308525 - Update activity stream panel to follow new UX mocks.

https://reviewboard.mozilla.org/r/85242/#review84614

LGTM, I'll read up a bit more on layouts and look through this stuff again, but I trust that it all works :)

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStream.java:70
(Diff revision 1)
>       * Extract a label from a URL to use in Activity Stream.
>       *
>       * This method implements the proposal from this desktop AS issue:
>       * https://github.com/mozilla/activity-stream/issues/1311
>       */
> -    public static String extractLabel(String url) {
> +    public static String extractLabel(String url, boolean usePath) {

Perhaps if usePath=true is pretty much your "default" value, adding a `extractLabel(String url) { extractLabel(url, true); }` method is worth it?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:87
(Diff revision 1)
>  
> +    @Override
> +    protected void onSizeChanged(int w, int h, int oldw, int oldh) {
> +        super.onSizeChanged(w, h, oldw, oldh);
> +
> +        int tiles = (w - tileMargin) / (desiredTileWidth + tileMargin);

That's nice, cleaner than trying to explicitely account for margins on either side of the tile :)

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:90
(Diff revision 1)
> +        super.onSizeChanged(w, h, oldw, oldh);
> +
> +        int tiles = (w - tileMargin) / (desiredTileWidth + tileMargin);
> +
> +        if (tiles < MINIMUM_TILES) {
> +            tiles = MINIMUM_TILES;

If there isn't enough space, wouldn't you want to have less tiles?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:98
(Diff revision 1)
> +        } else if (tiles > MAXIMUM_TILES) {
> +            tiles = MAXIMUM_TILES;
> +
> +            // Use the remaining space as padding
> +            int needed = tiles * (desiredTileWidth + tileMargin) + tileMargin;
> +            int padding = (w - needed) / 2;

Couldn't this end up being a negative value, depending on what your desiredTileWidth is in relation to w?

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamItem.java:86
(Diff revision 1)
> +        private int tilesMargin;
>  
>          public HighlightItem(View itemView) {
>              super(itemView);
> +
> +            tilesMargin = (int) TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 10, itemView.getContext().getResources().getDisplayMetrics());

Did you mean to use `activity_stream_base_margin` value?

::: mobile/android/base/resources/layout/activity_stream_card_history_item.xml:49
(Diff revision 1)
> +
> +        <TextView
> +            android:id="@+id/page"
> +            android:layout_width="match_parent"
> +            android:layout_height="wrap_content"
> +            tools:text="twitter"

placeholder?

::: mobile/android/base/resources/layout/activity_stream_card_history_item.xml:75
(Diff revision 1)
>              android:textStyle="bold"
>              android:textColor="#ff000000"
> -            tools:text="Descriptive title of a page..." />
> +            android:layout_below="@id/page"
> +            android:layout_toLeftOf="@id/menu"
> +            android:layout_toStartOf="@id/menu"
> +            tools:text="Descriptive title of a page that is veeeeeeery long - maybe even too long?" />

placeholder?

::: mobile/android/base/resources/layout/activity_stream_topsites_card.xml:45
(Diff revision 1)
>              android:gravity="center"
>              android:lines="1"
>              android:padding="4dp"
> +            android:textSize="12sp"
>              android:textColor="@android:color/black"
> -            tools:text="Lorem Ipsum here is a title"
> +            tools:text="Lorem Ipsum here is a title" />

I guess there are a lot of placeholders for now :)

::: mobile/android/base/resources/values/dimens.xml:222
(Diff revision 1)
>  
>      <item name="notification_media_cover" type="dimen">128dp</item>
> +
> +    <item name="activity_stream_base_margin" type="dimen">10dp</item>
> +    <item name="activity_stream_desired_tile_width" type="dimen">90dp</item>
> +    <item name="activity_stream_desired_tile_height" type="dimen">70dp</item>

Proportionally tile height/width are a bit off from what the spec is (ratios of 0.77 vs 0.89). I guess that's on purpose?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8800688 [details]
Bug 1308525 - Update activity stream panel to follow new UX mocks.

https://reviewboard.mozilla.org/r/85242/#review84614

> If there isn't enough space, wouldn't you want to have less tiles?

We have two options: Either have less tiles or have smaller tiles. In this case we decided to have 4 tiles as minimum and therefore accept to have tiles smaller than the desired tiles if they do not fit.

> Couldn't this end up being a negative value, depending on what your desiredTileWidth is in relation to w?

This is for the case where we theoretically could fit more than 6 tiles. So there's at least room for one more tile -> After calculating the space for the maximum of 6 tiles there should always be space left that we will use as padding on the left and right (This is for tablets).

> placeholder?

Yeah, tools:text is just for displaying this text in the UI builder/preview.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8800688 [details]
Bug 1308525 - Update activity stream panel to follow new UX mocks.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/738471dfbee1
Update activity stream panel to follow new UX mocks. r=Grisha

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/738471dfbee1