Closed
Bug 1379021
Opened 7 years ago
Closed 7 years ago
UI update for the Fennec New Tab Page - Activity Stream
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, enhancement, P1)
Firefox for Android Graveyard
Theme and Visual Design
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bbell, Assigned: sebastian)
References
()
Details
(Whiteboard: [MobileAS])
Attachments
(5 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mcomella
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Details |
Mockup for the New Tab UI uplift. https://mozilla.invisionapp.com/share/5JCHCGXPG#/224085513_New_Tab_-_Keyboard
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [MobileAS] → [MobileAS]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Started with refactoring the highlights. Not complete yet. :)
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Priority: P2 → P1
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 hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8886168 [details] Bug 1379021 - Remove card wrapper from highlight items. https://reviewboard.mozilla.org/r/156960/#review164284
Attachment #8886168 -
Flags: review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8886169 [details] Bug 1379021 - Remove divider from highlights title layout. https://reviewboard.mozilla.org/r/156962/#review164288
Attachment #8886169 -
Flags: review+
Comment 16•7 years ago
|
||
mozreview-review |
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 17•7 years ago
|
||
mozreview-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 18•7 years ago
|
||
mozreview-review-reply |
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 19•7 years ago
|
||
mozreview-review |
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 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8887993 [details] Bug 1379021 - Remove pager indicator from top sites. https://reviewboard.mozilla.org/r/158866/#review164304
Attachment #8887993 -
Flags: review+
Comment 21•7 years ago
|
||
mozreview-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+
Attachment #8886168 -
Flags: review?(gkruglov)
Attachment #8886169 -
Flags: review?(gkruglov)
Attachment #8886170 -
Flags: review?(gkruglov)
Attachment #8886171 -
Flags: review?(gkruglov)
Attachment #8886172 -
Flags: review?(gkruglov)
Attachment #8887993 -
Flags: review?(gkruglov)
Attachment #8887994 -
Flags: review?(gkruglov)
Assignee | ||
Comment 22•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49580bff7d365ad8c4ac92541b4d17b409c96c79
Assignee | ||
Updated•7 years ago
|
Whiteboard: [MobileAS] → [MobileAS] 1.26
Updated•7 years ago
|
Iteration: --- → 1.26
Assignee | ||
Updated•7 years ago
|
Whiteboard: [MobileAS] 1.26 → [MobileAS]
Assignee | ||
Comment 23•7 years ago
|
||
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) |
Assignee | ||
Updated•7 years ago
|
Attachment #8886171 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8886172 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 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•7 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•7 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
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d18e2a7fb353 https://hg.mozilla.org/mozilla-central/rev/55644d60b1cd https://hg.mozilla.org/mozilla-central/rev/ddb3af69403c https://hg.mozilla.org/mozilla-central/rev/21ac985081fd https://hg.mozilla.org/mozilla-central/rev/561a65c7698f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•