Activity Stream: Update measurements and highlights style

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

unspecified
Firefox 52
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8798892 [details]
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.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1306608
See Also: → bug 1309342
(Assignee)

Updated

2 years ago
Depends on: 1309821
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
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+
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Depends on: 1310603

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/738471dfbee1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(Assignee)

Updated

2 years ago
Blocks: 1311099

Updated

2 years ago
Iteration: --- → 1.7

Updated

a year ago
See Also: → bug 1310143
You need to log in before you can comment on or make changes to this bug.