Closed Bug 1393174 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
1.30
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified

People

(Reporter: mpopova, Assigned: liuche)

References

Details

(Whiteboard: [mobileAS])

Attachments

(4 files)

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.
tracking-fennec: --- → ?
Whiteboard: [mobileAS]
tracking-fennec: ? → +
Priority: -- → P2
Rank: 1
This needs UX - something small would just be adding a link somewhere that takes users to the Pocket Trending stories page.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
let's add a simple "more" button to the Pocket header that links to Pocket's (Mobile) Trending Content Page. 

See example

Color: #008EA4
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
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
Flags: needinfo?(mpopova)
Flags: needinfo?(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.
Flags: needinfo?(csadilek)
Flags: needinfo?(mpopova)
Rank: 1 → 2
Dropping this to P2 rank 2 for the new merge date.
Assignee: nobody → liuche
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?)
Attachment #8908437 - Flags: review?(michael.l.comella) → review+
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. :(
Attachment #8908438 - Flags: review?(michael.l.comella) → review+
Iteration: --- → 1.30
Rank: 2
Priority: P2 → P1
Blocks: 1400408
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 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!
Pushed by cliu@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/82b8495eb44f
https://hg.mozilla.org/mozilla-central/rev/49dd4d0c6d0e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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?
Flags: needinfo?(liuche)
(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.
Okay, thanks flod, I filed bug 1400950 to fix that.
Flags: needinfo?(liuche)
More button has been added and works as expected, marking this as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.