1393174 - Provide a mechanism (link) to load more Pocket Stories on new tab

Status: VERIFIED FIXED

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

Description

[Maria Popova]

Currently, only 3 stories are loaded on a new tab.  Up to 10 stories can be fetched, so we need a way to allow the user to load/explore more Pocket stories.

Comment 1

[Chenxia Liu [:liuche]]

This needs UX - something small would just be adding a link somewhere that takes users to the Pocket Trending stories page.

Comment 2

[bbell]

Attachment: New Tab - Pocket [e-mail]

let's add a simple "more" button to the Pocket header that links to Pocket's (Mobile) Trending Content Page. 

See example

Color: #008EA4

Comment 3

[Chenxia Liu [:liuche]]

Following up here: Christian is there a mobile-optimized "Pocket Trending Stories" link that you'd like us to link out to from the new tab page? Otherwise, we'll just use the Desktop one, with the same referer id: https://getpocket.cdn.mozilla.net/explore/trending?src=ff_new_tab

Comment 4

[Christian Sadilek [:csadilek]]

(In reply to Chenxia Liu [:liuche] from comment #3)
> Following up here: Christian is there a mobile-optimized "Pocket Trending
> Stories" link that you'd like us to link out to from the new tab page?
> Otherwise, we'll just use the Desktop one, with the same referer id:
> https://getpocket.cdn.mozilla.net/explore/trending?src=ff_new_tab

Hi, Just clarified with Matt. We should be using src=ff_android and src=ff_ios respectively. Thanks.

Comment 5

[Chenxia Liu [:liuche]]

Dropping this to P2 rank 2 for the new merge date.

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Screenshot: post-patch "MORE" link

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Bug 1393174 - Add "More" link to Pocket AS title.

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

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Bug 1393174 - Rename FirstrunRegular.Link to Link.

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

Comment 9

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

Comment on attachment 8908437 [details]
Bug 1393174 - Add "More" link to Pocket AS title.

https://reviewboard.mozilla.org/r/180080/#review185494

Looks good though I'm concerned this could break 1) in RTL (when we have right-aligned text) and 2) when we disable Pocket.

::: mobile/android/app/src/main/res/layout/activity_stream_main_highlightstitle.xml:1
(Diff revision 1)
>  <?xml version="1.0" encoding="utf-8"?>

Could we rename this file so that it's no longer about highlights and it's about stream title row?

::: mobile/android/app/src/main/res/layout/activity_stream_main_highlightstitle.xml:21
(Diff revision 1)
> -    android:layout_marginTop="@dimen/activity_stream_base_margin"
> -    android:layout_marginBottom="@dimen/activity_stream_base_margin"
> +        android:layout_marginBottom="@dimen/activity_stream_base_margin"
> -    android:layout_marginRight="@dimen/activity_stream_base_margin"
> +        android:layout_marginRight="@dimen/activity_stream_base_margin"
> -    android:layout_marginEnd="@dimen/activity_stream_base_margin"
> +        android:layout_marginEnd="@dimen/activity_stream_base_margin"
> -    android:layout_width="match_parent"
> -    android:layout_height="wrap_content"
> +        android:layout_marginStart="@dimen/activity_stream_base_margin"
> +        android:layout_marginTop="@dimen/activity_stream_base_margin"

nit: You're declaring margins for all directions with the same value so you could use the `layout_margin` tag.

::: mobile/android/app/src/main/res/layout/activity_stream_main_highlightstitle.xml:26
(Diff revision 1)
> -    android:layout_height="wrap_content"
> -    android:textStyle="bold"
> +        android:layout_marginTop="@dimen/activity_stream_base_margin"
> +        android:textColor="#FF858585"
> -    android:textSize="16sp"
> +        android:textSize="16sp"
> -    android:textColor="#FF858585" />
> +        android:textStyle="bold" />
> +
> +    <TextView

nit: add a comment explaining that we show these views in code if they're relevant to the title

::: mobile/android/app/src/main/res/layout/activity_stream_main_highlightstitle.xml:30
(Diff revision 1)
> +
> +    <TextView
> +        android:id="@+id/title_link"
> +        android:layout_width="wrap_content"
> +        android:layout_height="wrap_content"
> +        android:layout_gravity="center_vertical|start|left"

nit: I think you can start without left for these flags (iirc, Android Studio tells you that start is a superset of the bit flags of left so it works on all platforms).

That being said, why is start necessary? I think these views will fill the parent horizontally so the placement of this view at start doesn't mean anything.

::: mobile/android/app/src/main/res/layout/activity_stream_main_highlightstitle.xml:31
(Diff revision 1)
> +    <TextView
> +        android:id="@+id/title_link"
> +        android:layout_width="wrap_content"
> +        android:layout_height="wrap_content"
> +        android:layout_gravity="center_vertical|start|left"
> +        android:paddingEnd="@dimen/activity_stream_base_margin"

I think you need to specify Left/Start = 0dp

::: mobile/android/app/src/main/res/layout/activity_stream_main_highlightstitle.xml:33
(Diff revision 1)
> +        android:layout_width="wrap_content"
> +        android:layout_height="wrap_content"
> +        android:layout_gravity="center_vertical|start|left"
> +        android:paddingEnd="@dimen/activity_stream_base_margin"
> +        android:paddingRight="@dimen/activity_stream_base_margin"
> +        android:textAppearance="@style/TextAppearance.FirstrunRegular.Link"

nit: if we're using this style in more places than FirstRun, I think we should rename it so that it's more generic and apparent that it's being used outside first run.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:24
(Diff revision 1)
> +import java.util.EnumSet;
>  
>  public class StreamTitleRow extends StreamViewHolder {
>      public static final int LAYOUT_ID = R.layout.activity_stream_main_highlightstitle;
>  
> +

nit: ws

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:34
(Diff revision 1)
>          if (!isEnabled) {
>              hideView(itemView);
>          }
>      }
>  
> +    public StreamTitleRow(final View itemView, final @StringRes @NonNull int titleResId, boolean isEnabled, final int linkTitleResId, final String url, final HomePager.OnUrlOpenListener onUrlOpenListener) {

nit: wrap

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:34
(Diff revision 1)
>          if (!isEnabled) {
>              hideView(itemView);
>          }
>      }
>  
> +    public StreamTitleRow(final View itemView, final @StringRes @NonNull int titleResId, boolean isEnabled, final int linkTitleResId, final String url, final HomePager.OnUrlOpenListener onUrlOpenListener) {

nit: `linkTitleResId` can also get `@StringRes`

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:37
(Diff revision 1)
>      }
>  
> +    public StreamTitleRow(final View itemView, final @StringRes @NonNull int titleResId, boolean isEnabled, final int linkTitleResId, final String url, final HomePager.OnUrlOpenListener onUrlOpenListener) {
> +        this(itemView, titleResId, isEnabled);
> +        // Android 21+ is needed to set RTL-aware compound drawables, so we use a tinted ImageView here.
> +        final TextView titleLink = (TextView) itemView.findViewById(R.id.title_link);

What will happen if we disable Pocket? Do we need to hide these views if the TitleRow is not enabled, like the other constructor? (though I think the StreamRecyclerAdapter should actually own changing visibility of this container based on view enabled state)

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:40
(Diff revision 1)
> +        this(itemView, titleResId, isEnabled);
> +        // Android 21+ is needed to set RTL-aware compound drawables, so we use a tinted ImageView here.
> +        final TextView titleLink = (TextView) itemView.findViewById(R.id.title_link);
> +        titleLink.setVisibility(View.VISIBLE);
> +        titleLink.setText(linkTitleResId);
> +        titleLink.setClickable(true);

nit: `setClickable` does not appear necessary, here and below.

The framework does it for you when calling `setOnClickListener`: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/view/View.java#5488

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:50
(Diff revision 1)
> +        titleArrow.setClickable(true);
> +
> +        final View.OnClickListener clickListener = new View.OnClickListener() {
> +            @Override
> +            public void onClick(View view) {
> +                onUrlOpenListener.onUrlOpen(url, EnumSet.of(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));

Did we want to add telemetry? (follow-up?)

Comment 10

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

Comment on attachment 8908438 [details]
Bug 1393174 - Rename FirstrunRegular.Link to Link.

https://reviewboard.mozilla.org/r/180082/#review185510

Oh great, you already fixed it. :)

I hate these styles that inherit from empty styles just so the name is pretty. :(

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8908437 [details]
Bug 1393174 - Add "More" link to Pocket AS title.

https://reviewboard.mozilla.org/r/180080/#review185494

> What will happen if we disable Pocket? Do we need to hide these views if the TitleRow is not enabled, like the other constructor? (though I think the StreamRecyclerAdapter should actually own changing visibility of this container based on view enabled state)

The visibility of these views are set when the view is created so it's only created when the Pocket title is needed. You can also see that the title resid is set at the same time so this is only made visible on creation - we don't have any problems with the title being "Pocket" when it should be "Highlights" so there shouldn't be any problems here either.

And yes, ideally the Adapter should handle binding these views (and this would help with the settings "refresh on settings change" bug as well).

> Did we want to add telemetry? (follow-up?)

Filed bug 1400408.

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8908437 [details]
Bug 1393174 - Add "More" link to Pocket AS title.

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

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8908438 [details]
Bug 1393174 - Rename FirstrunRegular.Link to Link.

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

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8908437 [details]
Bug 1393174 - Add "More" link to Pocket AS title.

https://reviewboard.mozilla.org/r/180080/#review185494

I don't think 2) is a problem, and I checked 1) with RTL - while the "Recommended By Pocket" title isn't justified correctly, I think it's because I don't have a multilocale build and it matches the "Highlights" title view, and the "MORE >" views are in the right place.

I'll double check to see how this works in Nightly!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82b8495eb44f
Add "More" link to Pocket AS title. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/49dd4d0c6d0e
Rename FirstrunRegular.Link to Link. r=mcomella

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/82b8495eb44f
https://hg.mozilla.org/mozilla-central/rev/49dd4d0c6d0e

Comment 17

[Francesco Lodolo [:flod]]

https://hg.mozilla.org/mozilla-central/rev/82b8495eb44f
Besides the comment misspelled (NOET)

+<!-- LOCALIZATION NOET (activity_stream_link_more): Link-like text displayed to take user to a website with more content from Pocket. This string will be displayed in all uppercase. -->

If you want this to be MORE, please put MORE directly as string. But, I don't see anything in the code changing the case to uppercase, what am I missing?

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #17)
> If you want this to be MORE, please put MORE directly as string. But, I
> don't see anything in the code changing the case to uppercase, what am I
> missing?

Also note that this string is already exposed to localizers, changing it would require a new string ID.

Comment 19

[Chenxia Liu [:liuche]]

Okay, thanks flod, I filed bug 1400950 to fix that.

Comment 20

[Bogdan Surd, QA [:bsurd, NI]]

More button has been added and works as expected, marking this as verified.