1400950 - Activity Stream section titles styling

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Chenxia Liu [:liuche]]

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.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Screenshot: styling changes listed in bug

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Bug 1400950 - Update string for "MORE" link to be caps.

Review commit: https://reviewboard.mozilla.org/r/181558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181558/

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Bug 1400950 - Update title font size and spacing.

Review commit: https://reviewboard.mozilla.org/r/181560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181560/

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Bug 1400950 - Update AS panel color.

Review commit: https://reviewboard.mozilla.org/r/181562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181562/

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Bug 1400950 - Don't draw divider above Highlights title.

Review commit: https://reviewboard.mozilla.org/r/181564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181564/

Comment 6

[Chenxia Liu [:liuche]]

bbell made the changes you requested (see screenshot), lmk if there's anything else you want!

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8910075 [details]
Bug 1400950 - Update string for "MORE" link to be caps.

https://reviewboard.mozilla.org/r/181558/#review187102

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8910076 [details]
Bug 1400950 - Update title font size and spacing.

https://reviewboard.mozilla.org/r/181560/#review187104

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 +).

Comment 11

[Chenxia Liu [:liuche]]

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 12

[Chenxia Liu [:liuche]]

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!

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8910078 [details]
Bug 1400950 - Don't draw divider above Highlights title.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181564/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[bbell]

looks good, thanks!

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ef94a037bab9
https://hg.mozilla.org/mozilla-central/rev/fbd44d38b79e
https://hg.mozilla.org/mozilla-central/rev/517ccb27a05c
https://hg.mozilla.org/mozilla-central/rev/802042e3b5e0