Closed Bug 1379021 Opened 8 years ago Closed 8 years ago

UI update for the Fennec New Tab Page - Activity Stream

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Theme and Visual Design
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bbell, Assigned: sebastian)

References

(
URL
)

Details

(Whiteboard: [MobileAS])

Attachments

(5 files, 2 obsolete files)

Bug 1379021 - Remove card wrapper from highlight items.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1379021 - Remove divider from highlights title layout.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1379021 - Draw a horizontal divider between highlight items.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1379021 - Remove pager indicator from top sites.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1379021 - Update top sites layout to match updated UI mocks.
59 bytes, text/x-review-board-request
Details
Blocks: 1377290
Whiteboard: [MobileAS]
Priority: -- → P2
Whiteboard: [MobileAS] → [MobileAS]
Depends on: 1380639
Started with refactoring the highlights. Not complete yet. :)
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Priority: P2 → P1
Blocks: 1381524
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.
Attachment #8886171 - Attachment is obsolete: true
Attachment #8886172 - Attachment is obsolete: true
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.
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. :)
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: