Closed Bug 1308525 Opened 5 years ago Closed 5 years ago

Activity Stream: Update measurements and highlights style

Categories

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

All
Android
defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

(Whiteboard: [MobileAS])

Attachments

(2 files)

Attached image 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.
Duplicate of this bug: 1306608
See Also: → 1309342
Depends on: 1309821
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?
Attachment #8800688 - Flags: review?(gkruglov) → review+
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.
Depends on: 1310603
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/738471dfbee1
Update activity stream panel to follow new UX mocks. r=Grisha
https://hg.mozilla.org/mozilla-central/rev/738471dfbee1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Iteration: --- → 1.7
See Also: → 1310143
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.