Activity Stream section titles styling

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

Trunk
Firefox 57
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [MobileAS])

Attachments

(5 attachments)

From bbell:

- Section titles should be 18 instead of 16
- Decrease spacing between "MORE" and ">"
- Remove spacer line above "Highlights"
- Background color should be photon-10 #F9F9FA

And for localization, the "MORE" should be capitalized in the string itself, rather than capitalized in-code.
Assignee: nobody → liuche
bbell made the changes you requested (see screenshot), lmk if there's anything else you want!
Flags: needinfo?(bbell)
Comment on attachment 8910075 [details]
Bug 1400950 - Update string for "MORE" link to be caps.

https://reviewboard.mozilla.org/r/181558/#review187102
Attachment #8910075 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8910076 [details]
Bug 1400950 - Update title font size and spacing.

https://reviewboard.mozilla.org/r/181560/#review187104
Attachment #8910076 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8910077 [details]
Bug 1400950 - Update AS panel color.

https://reviewboard.mozilla.org/r/181562/#review187112

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:64
(Diff revision 1)
>      private final SharedPreferences sharedPreferences;
>  
>      public ActivityStreamPanel(Context context, AttributeSet attrs) {
>          super(context, attrs);
>  
> -        setBackgroundColor(ContextCompat.getColor(context, R.color.about_page_header_grey));
> +        setBackgroundColor(ContextCompat.getColor(context, R.color.photon_browser_toolbar_bg));

nit: I don't know what our current color conventions are but I dislike the idea of using a color that's explicitly named to be used somewhere (the toolbar) in another location (which is why I introduced the neutral color palette in the first place).

But since we no longer follow the color palette convention (argh!), maybe this is the expected convention?
Attachment #8910077 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8910078 [details]
Bug 1400950 - Don't draw divider above Highlights title.

https://reviewboard.mozilla.org/r/181564/#review187114

I think this code is trying to do two different things so while it may work, it's fragile. The calculation for the divider top is also incorrect, but I think it shold be easy to fix. Since we're close to the merge, I may not have enough time to review and I trust you to fix it correctly so r+.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:20
(Diff revision 1)
>  
>  /**
>   * ItemDecoration implementation that draws horizontal divider line between highlight items.
>   */
>  /* package */ class HighlightsDividerItemDecoration extends RecyclerView.ItemDecoration {
>      // We do not want to draw a divider for the Top Sites panel.

nit: do we need to update this comment?

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:51
(Diff revision 1)
>  
>              if (child.getVisibility() == View.GONE) {
>                  continue;
>              }
>  
> +            if (parent.getAdapter().getItemViewType(i) == StreamRecyclerAdapter.RowItemType.HIGHLIGHTS_TITLE.getViewType()) {

Why is this necessary? Shouldn't it be taken care of with `START_DRAWING_AT_POSITION`, like we've done in the past?

Is it because of the `top =` change below?

In any case, I'd prefer not to mix paradigms here (if viewType, continue & if start_drawing_at_position, continue).

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:57
(Diff revision 1)
> +                continue;
> +            }
> +
>              final RecyclerView.LayoutParams params = (RecyclerView.LayoutParams) child
>                      .getLayoutParams();
> -            final int top = child.getBottom() + params.bottomMargin;
> +            final int top = child.getTop() + params.topMargin;

I think `top` actually refers to the top boundary of the divider, not the top boundary of the view (it should probably be called `topOfTheDivider` or similar).

So `top = child.getBottom() + params.bottomMargin` was fine, assuming we wanted to draw dividers below views rather than above them.

If you actually want to draw a divider on the top, I think it should be `top = child.getTop() - params.topMargin` (notice the - instead of the +).
Attachment #8910078 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8910077 [details]
Bug 1400950 - Update AS panel color.

https://reviewboard.mozilla.org/r/181562/#review187214

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:64
(Diff revision 1)
>      private final SharedPreferences sharedPreferences;
>  
>      public ActivityStreamPanel(Context context, AttributeSet attrs) {
>          super(context, attrs);
>  
> -        setBackgroundColor(ContextCompat.getColor(context, R.color.about_page_header_grey));
> +        setBackgroundColor(ContextCompat.getColor(context, R.color.photon_browser_toolbar_bg));

Yeah, I wasn't sure what this should be, but I'll ask bbell. I assume they're meant to match? so I didn't worry too much about it.
Comment on attachment 8910078 [details]
Bug 1400950 - Don't draw divider above Highlights title.

https://reviewboard.mozilla.org/r/181564/#review187216

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:20
(Diff revision 1)
>  
>  /**
>   * ItemDecoration implementation that draws horizontal divider line between highlight items.
>   */
>  /* package */ class HighlightsDividerItemDecoration extends RecyclerView.ItemDecoration {
>      // We do not want to draw a divider for the Top Sites panel.

This is a change to make the dividers draw above rather than below, so I guess the result is the same, but I can update it (and also the class comment actually, which is not just highlights anymore).

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:51
(Diff revision 1)
>  
>              if (child.getVisibility() == View.GONE) {
>                  continue;
>              }
>  
> +            if (parent.getAdapter().getItemViewType(i) == StreamRecyclerAdapter.RowItemType.HIGHLIGHTS_TITLE.getViewType()) {

That's fair - basically we do not want to draw dividers above the title sections, so I'll add a case to check for either section.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:57
(Diff revision 1)
> +                continue;
> +            }
> +
>              final RecyclerView.LayoutParams params = (RecyclerView.LayoutParams) child
>                      .getLayoutParams();
> -            final int top = child.getBottom() + params.bottomMargin;
> +            final int top = child.getTop() + params.topMargin;

Good catch!
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef94a037bab9
Update string for "MORE" link to be caps. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/fbd44d38b79e
Update title font size and spacing. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/517ccb27a05c
Update AS panel color. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/802042e3b5e0
Don't draw divider above Highlights title. r=mcomella
looks good, thanks!
Flags: needinfo?(bbell)
You need to log in before you can comment on or make changes to this bug.