UI update for the Fennec New Tab Page - Activity Stream

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbell, Assigned: sebastian)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [MobileAS], )

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Updated

2 years ago
Blocks: 1377290
Reporter

Updated

2 years ago
Whiteboard: [MobileAS]
Priority: -- → P2
Whiteboard: [MobileAS] → [MobileAS]
Depends on: 1380639
Comment hidden (mozreview-request)
Started with refactoring the highlights. Not complete yet. :)

Updated

2 years ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Priority: P2 → P1
Blocks: 1381524
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8886170 [details]
Bug 1379021 - Draw a horizontal divider between highlight items.

https://reviewboard.mozilla.org/r/156964/#review164290

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:19
(Diff revision 2)
> +import android.view.View;
> +
> +/**
> + * ItemDecoration implementation that draws horizontal divider line between highlight items.
> + */
> +/* package */ class HighlightsDividerItemDecoration extends RecyclerView.ItemDecoration {

nit: Do we have a convention for `/* package */`?

I personally don't find much of a difference between `package-private` and `protected` for applications so I find the markings redundant.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:26
(Diff revision 2)
> +    private static final int START_DRAWING_AT_POSITION = 2;
> +
> +    private static final int[] ATTRS = new int[]{
> +            android.R.attr.listDivider
> +    };
> +    private Drawable mDivider;

nit: -> `divider`
Attachment #8886170 - Flags: review+
Comment on attachment 8886171 [details]
Bug 1379021 - Rebuild highlights row layout based on ConstraintLayout.

https://reviewboard.mozilla.org/r/156966/#review164296

Not sure if my comments are relevant: ConstraintLayout may do some funky shiz and that could be okay - you tell me! :)

::: mobile/android/app/src/main/res/layout/activity_stream_highlights_item.xml:15
(Diff revision 2)
> +    android:layout_height="wrap_content"
> +    android:background="?android:attr/selectableItemBackground"
> +    android:paddingBottom="8dp">
> +
> +    <FrameLayout
> +        android:id="@+id/icon_wrapper"

Why is this view necessary? Why not add margins to the FaviconView? Add a comment.

::: mobile/android/app/src/main/res/layout/activity_stream_highlights_item.xml:16
(Diff revision 2)
> +    android:background="?android:attr/selectableItemBackground"
> +    android:paddingBottom="8dp">
> +
> +    <FrameLayout
> +        android:id="@+id/icon_wrapper"
> +        android:layout_width="100dp"

Why these hard-coded bounds? Add a comment.
Attachment #8886171 - Flags: review+
Comment on attachment 8886171 [details]
Bug 1379021 - Rebuild highlights row layout based on ConstraintLayout.

https://reviewboard.mozilla.org/r/156966/#review164296

> Why is this view necessary? Why not add margins to the FaviconView? Add a comment.

I see it's removed in the next commit.
Comment on attachment 8886172 [details]
Bug 1379021 - Remove code to resize highlights tiles dynamically.

https://reviewboard.mozilla.org/r/156968/#review164300

::: mobile/android/app/src/main/res/layout/activity_stream_highlights_item.xml:1
(Diff revision 2)
> -<?xml version="1.0" encoding="utf-8"?>
> +<?xml version="1.0" encoding="utf-8"?><!-- This Source Code Form is subject to the terms of the Mozilla Public

nit: should be on a newline
Attachment #8886172 - Flags: review+
Comment on attachment 8887994 [details]
Bug 1379021 - Update top sites layout to match updated UI mocks.

https://reviewboard.mozilla.org/r/158868/#review164306

Didn't dive in super deep (in particular to better understand what exists and how it's changing) but seems reasonable.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java:102
(Diff revision 1)
>                  .build()
>                  .execute(this);
>  
>          final Drawable pinDrawable;
>          if (topSite.isPinned()) {
> -            pinDrawable = DrawableUtil.tintDrawable(itemView.getContext(), R.drawable.as_pin, itemView.getResources().getColor(R.color.placeholder_grey));
> +            pinDrawable = DrawableUtil.tintDrawable(itemView.getContext(), R.drawable.as_pin, 0xFFFFFFFF);

I had no idea `0xFFFFFFFF` worked - are you sure you aren't experiencing undefined behavior and you should do `Color.parseColor("#FFFFFFFF");` instead?
Attachment #8887994 - Flags: review+
Whiteboard: [MobileAS] → [MobileAS] 1.26
Iteration: --- → 1.26
Whiteboard: [MobileAS] 1.26 → [MobileAS]
I'll go ahead and start landing some pieces that do not depend on Constraint Layout. The other pieces I'll move to new bugs attached to meta bug 1377290.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8886171 - Attachment is obsolete: true
Attachment #8886172 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Assignee

Comment 30

2 years ago
mozreview-review-reply
Comment on attachment 8886170 [details]
Bug 1379021 - Draw a horizontal divider between highlight items.

https://reviewboard.mozilla.org/r/156964/#review164290

> nit: Do we have a convention for `/* package */`?
> 
> I personally don't find much of a difference between `package-private` and `protected` for applications so I find the markings redundant.

I tend to give classes/properties the lowest visibility possible. Here I want to use the class from within the package (a lot here is only shared in this package and not for consumption in the rest of the app). I do not use 'protected' or higher to not falsely imply that I expect this to be used more broadly.
Assignee

Comment 31

2 years ago
mozreview-review-reply
Comment on attachment 8887994 [details]
Bug 1379021 - Update top sites layout to match updated UI mocks.

https://reviewboard.mozilla.org/r/158868/#review164306

> I had no idea `0xFFFFFFFF` worked - are you sure you aren't experiencing undefined behavior and you should do `Color.parseColor("#FFFFFFFF");` instead?

At the end of the day Color.parse() will create exactly the same int again:
https://github.com/android/platform_frameworks_base/blob/master/graphics/java/android/graphics/Color.java#L146

But I guess Color.WHITE is something I should use here. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

2 years ago
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d18e2a7fb353
Remove card wrapper from highlight items. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/55644d60b1cd
Remove divider from highlights title layout. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/ddb3af69403c
Draw a horizontal divider between highlight items. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/21ac985081fd
Remove pager indicator from top sites. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/561a65c7698f
Update top sites layout to match updated UI mocks. r=mcomella
No longer depends on: 1380639
You need to log in before you can comment on or make changes to this bug.