Fix Bug 1432657 - Add pocket card type to highlight types #3975

sarracini · 2018-02-07T16:05:41Z

Allows Pocket items to be shown in highlights
Adds a "remove from pocket list" option in context menu (includes string + icon)
Adds functionality to actually remove the item from the pocket list via context menu
Adds a "saved to pocket" highlight context type (includes string + small version of regular pocket icon)
Preemptively updates UI when removing a pocket item via the reducer

Has the same behaviour as when you bookmark a trending story - update the card itself then next time you refresh it shows it in the highlights section

Mardak

Seems like there's some open design questions as well as checking with pocket people on the behaviors / consistent UX across their products.

Mardak · 2018-02-13T21:57:30Z

locales/en-US/strings.properties

@@ -54,6 +55,7 @@ confirm_history_delete_p1=Are you sure you want to delete every instance of this
 # page from history.
 confirm_history_delete_notice_p2=This action cannot be undone.
 menu_action_save_to_pocket=Save to Pocket
+menu_action_remove_pocket=Remove from Pocket List


Probably confirm with pocket(?) about this string. In particular the opposite string is just "Save to Pocket" (no "List")

Designers said "Delete on Pocket" and "Archive on Pocket"

Mardak · 2018-02-13T21:57:30Z

locales/en-US/strings.properties

@@ -29,6 +29,7 @@ type_label_visited=Visited
 type_label_bookmarked=Bookmarked
 type_label_synced=Synced from another device
 type_label_recommended=Trending
+type_label_pocket=Saved To Pocket


Probably also confirm with pocket people on their terminology.

Noted on IRC that this should have lowercase "t" for "to"

Mardak · 2018-02-13T21:57:30Z

system-addon/lib/PlacesFeed.jsm

+    }
+    try {
+      // If the user is logged in, just save the link to Pocket and update the page
+      let {item_id} = await this.addLinkToPocket(url, title);


What is the expected behavior of saving to pocket? Before, it would open up the tagging doorhanger and trigger the library animation.

Note that the bookmark behavior is to open the bookmark doorhanger that triggers the library animation.

While we wait for the ui wanted for the correct behaviour, I'll explain my reasoning behind this change..
calling Pocket.savePage although does drop the doorhanger down doesn't have any indication of a successful or not successful save once it goes off and does it's stuff. With the pktApi we get the pocket_id back when we're successful so we can update the card we just saved, similar to how we do that for bookmark (i.e changing the context menu + highlight context) So I traded off one kind of confirmation feedback for a different one. Ideally we would call addLinkToPocket, and then also call something in Pocket.jsm to just show the doorhanger + library animation.

The designers say don't bother with the doorhanger :D

Mardak · 2018-02-13T21:57:30Z

system-addon/lib/HighlightsFeed.jsm

@@ -112,14 +111,20 @@ this.HighlightsFeed = class HighlightsFeed {
        this.fetchImage(page);
      }

+      // When we bookmark something it adds a history entry - so adjust the type


This comment is a bit misleading. The reason why the type switches to "bookmark" is that a highlight that comes in as history/visited might happen to be bookmarked, so show it as bookmarked even though the source was from history.

These were my steps to reproduce for this that caused me to write this comment (on a clean profile):

Bookmark a trending pocket story from the context menu

Open debugger and set a breakpoint in HighlightsFeed.jsm right after we dedupe against top sites

Force a refresh in Higlights (I dismissed another trending story)

Observe this.store.getState().TopSites.rows and see that the first item in the array is the bookmarked trending story with type 'history' ... even though I've never actually visited it.

So it's a bit confusing that it's there even though I never actually visited the page. How about "Adjust the type for 'history' items that are also 'bookmarked'" for the comment?

Mardak · 2018-02-13T21:57:30Z

system-addon/common/Reducers.jsm

@@ -289,6 +305,7 @@ function Sections(prevState = INITIAL_STATE.Sections, action) {
      return prevState.map(section => Object.assign({}, section,
        {rows: section.rows.filter(site => !action.data.includes(site.url))}));
    case at.PLACES_LINK_BLOCKED:
+    case at.REMOVE_FROM_POCKET:
      return prevState.map(section =>
        Object.assign({}, section, {rows: section.rows.filter(site => site.url !== action.data.url)}));


Hmm.. Removing a specific pocket item isn't the same as blocking a url.. where this code would hide any other bookmarks of the same url when unpocketing.

Mardak · 2018-02-13T21:57:30Z

system-addon/content-src/lib/link-menu-options.js

+    icon: "remove-pocket",
+    action: ac.AlsoToMain({
+      type: at.REMOVE_FROM_POCKET,
+      data: {pocket_id: site.pocket_id, url: site.url}


If we make the reducer remove by pocket_id then we won't need url here anymore.

sarracini · 2018-02-14T18:44:03Z

Notes from the UX meeting

Change the update to 1 hour instead of 5 minutes to not overload pocket servers
Make the URL it goes to be the pocket url not the resolved_url - pocket api not ready for this
Possibly add "archive" in context menu - pocket api also not ready for this

Mardak · 2018-02-16T21:37:53Z

system-addon/lib/HighlightsFeed.jsm

@@ -86,8 +89,7 @@ this.HighlightsFeed = class HighlightsFeed {

    // Request more than the expected length to allow for items being removed by
    // deduping against Top Sites or multiple history from the same domain, etc.
-    // Until bug 1425496 lands, do not include saved Pocket items in highlights
-    const manyPages = await this.linksCache.request({numItems: MANY_EXTRA_LENGTH, excludePocket: true});
+    const manyPages = await this.linksCache.request({numItems: MANY_EXTRA_LENGTH, excludePocket: options.excludePocket});


Mmm… Doesn't this mean we only include pocket items for the one refresh out of the ~12 each hour? So most highlight updates result in no pocket items shown?

Other way around, no? We want to update pocket on every refresh except the system_tick unless it's been over an hour... so if options.excludePocket is null for all refreshes, then !excludePocket should be true and then we'd go fetch them in NewTabUtils

Mardak

As discussed on irc, we'll want to add a pocket in-memory cache to NewTabUtils with various pocket wrappers (save, delete, archive) expiring the cache as well as expiring after an hour. HighlightsFeed will just always call without excludePocket.

sarracini · 2018-02-20T21:51:37Z

blocked on bug 1439684: https://bugzilla.mozilla.org/show_bug.cgi?id=1439684

Mardak · 2018-02-20T22:37:34Z

Given the Pocket team's desire to have sync instead of polling, we probably should have a pref that toggles excludePocket. We'll want it eventually anyway when we have Highlights sub option.

sarracini · 2018-02-21T16:54:00Z

system-addon/lib/ActivityStream.jsm

@@ -130,6 +130,10 @@ const PREFS_CONFIG = new Map([
    title: "Boolean flag that decides whether or not to show the topstories disclaimer.",
    value: true
  }],
+  ["section.highlights.excludePocket", {


Not sure about what namespace we want for the "sub-option" prefs

I guess it matches "section.highlights.collapsed" although we also have "browser.newtabpage.activity-stream.collapseTopSites"…

Let's keep it next to the other highlights pref. And perhaps rename it to "includePocket" as that's what the UI would expose.

sarracini · 2018-02-22T15:11:23Z

New design specs say pocket items should not be able to delete from history

sarracini · 2018-02-22T17:18:04Z

Blocked on: https://bugzilla.mozilla.org/show_bug.cgi?id=1440389

Mardak

Main thing is fixing up availableContextMenuOptions for stories too.

Mardak · 2018-02-22T22:55:52Z

system-addon/content-src/lib/link-menu-options.js

+  CheckPinTopSite: (site, index) => (site.isPinned ? LinkMenuOptions.UnpinTopSite(site) : LinkMenuOptions.PinTopSite(site, index)),
+  CheckSavedToPocket: (site, index) => (site.pocket_id ? LinkMenuOptions.DeleteFromPocket(site) : LinkMenuOptions.SaveToPocket(site, index)),
+  CheckBookmarkOrArchive: site => (site.pocket_id ? LinkMenuOptions.ArchiveFromPocket(site) : LinkMenuOptions.CheckBookmark(site)),
+  CheckDeleteHistoryOrEmpty: (site, index, eventSource) => (site.pocket_id ? LinkMenuOptions.EmptyItem() : LinkMenuOptions.DeleteUrl(site, index, eventSource))


Not sure if this is any better with a helper, so we can do something like…

{ CheckBookmark: check("bookmarkGuid", "RemoveBookmark", "AddBookmark"), CheckDeleteHistoryOrEmpty: check("pocket_id", "EmptyItem", "DeleteUrl") }

If we want to have the expressiveness of potentially checking all 3 site, index, eventSource (in the future?), the first argument to check could be a callback instead of just a string.

Mardak · 2018-02-22T22:55:52Z

locales/en-US/strings.properties

@@ -29,6 +29,7 @@ type_label_visited=Visited
 type_label_bookmarked=Bookmarked
 type_label_synced=Synced from another device
 type_label_recommended=Trending
+type_label_pocket=Saved To Pocket


Noted on IRC that this should have lowercase "t" for "to"

Mardak · 2018-02-22T22:55:52Z

system-addon/content-src/components/ContextMenu/ContextMenu.jsx

@@ -34,7 +34,7 @@ export class ContextMenu extends React.PureComponent {
      <ul role="menu" className="context-menu-list">
        {this.props.options.map((option, i) => (option.type === "separator" ?
          (<li key={i} className="separator" />) :
-          (<ContextMenuItem key={i} option={option} hideContext={this.hideContext} />)
+          (option.type !== "empty" && <ContextMenuItem key={i} option={option} hideContext={this.hideContext} />)


Seems like we want to have a switch (option.type) or .forEach instead of .map, but this is fine.

I'm assuming some of this is going to change anyways with weighted highlights, since we'll have to have a way to assign context menu options on a per card basis and we won't need this empty stuff anymore

Mardak · 2018-02-22T22:55:52Z

system-addon/lib/SectionsManager.jsm

@@ -66,7 +66,7 @@ const BUILT_IN_SECTIONS = {
    icon: "highlights",
    title: {id: "header_highlights"},
    maxRows: 3,
-    availableContextMenuOptions: ["CheckBookmark", "SaveToPocket", "Separator", "OpenInNewWindow", "OpenInPrivateWindow", "Separator", "BlockUrl", "DeleteUrl"],
+    availableContextMenuOptions: ["CheckBookmarkOrArchive", "CheckSavedToPocket", "Separator", "OpenInNewWindow", "OpenInPrivateWindow", "Separator", "BlockUrl", "CheckDeleteHistoryOrEmpty"],


We want these "Check" versions for topstories.availableContextMenuOptions too for the in-page update. (Fortunately, topstories already doesn't have "DeleteUrl" :p)

Mardak · 2018-02-22T22:55:52Z

system-addon/common/Reducers.jsm

+      }
+      return prevState.map(section => Object.assign({}, section, {
+        rows: section.rows.map(item => {
+          if (item.url === action.data.url) {


There's a slight bug / accidental(?) fix here in that if you pocket a story, stories' rows gets deduped via SECTION_UPDATE. A similar thing happens when bookmarking that @piatra and @csadilek worked on.

The bug is that even when Highlights no longer has the deduping url, e.g., archived pocket or unbookmarked page+delete history, rows is still deduped until it gets refreshed with an update or restart.

Simplest steps to reproduce:

save to pocket a story

archive that card

refresh page see that the card is still gone

toggle stories section from preferences, and see the story come back

Probably not really worth a fix as the user wanted it gone anyway and would most likely only run into it on restarting firefox, and by then, hopefully new stories would have come in…

Ah I see, yeah that's unfortunate but I think you're right, not super important to fix. Unless we added the saved to pocket trending card to the block list when they archive or save it or something. But probably not worth fixing right now

as-pine-proxy · 2018-03-06T22:06:04Z

Treeherder: 676200a7f29b2ccf29583c226d0edd1f1f49bd01

sarracini added the PR / In progress don't review yet label Feb 7, 2018

sarracini force-pushed the sarracini:bug_1432657 branch 5 times, most recently from 22867b3 to 1af05e2 Feb 8, 2018

sarracini removed the PR / In progress don't review yet label Feb 12, 2018

sarracini changed the title ~~[WIP] Bug 1432657 - Add pocket card type to highlight types~~ Bug 1432657 - Add pocket card type to highlight types Feb 12, 2018

sarracini requested a review from Mardak Feb 12, 2018

sarracini assigned Mardak Feb 12, 2018

sarracini added the PR / Needs review label Feb 12, 2018

Mardak changed the title ~~Bug 1432657 - Add pocket card type to highlight types~~ Fix Bug 1432657 - Add pocket card type to highlight types Feb 13, 2018

Mardak reviewed Feb 13, 2018

View changes

sarracini force-pushed the sarracini:bug_1432657 branch from 1af05e2 to 23e6b55 Feb 15, 2018

Mardak assigned sarracini and unassigned Mardak Feb 16, 2018

Mardak added PR / In progress don't review yet and removed PR / Needs review labels Feb 16, 2018

sarracini force-pushed the sarracini:bug_1432657 branch from 23e6b55 to 05b8905 Feb 16, 2018

Mardak reviewed Feb 16, 2018

View changes

Mardak suggested changes Feb 17, 2018

View changes

sarracini force-pushed the sarracini:bug_1432657 branch from 05b8905 to b6f93e9 Feb 20, 2018

sarracini reviewed Feb 21, 2018

View changes

sarracini force-pushed the sarracini:bug_1432657 branch 3 times, most recently from 90e7d15 to 3f3112d Feb 21, 2018

Mardak approved these changes Feb 22, 2018

View changes

sarracini added PR / Needs work and removed PR / In progress don't review yet labels Feb 23, 2018

Bug 1432657 - Add pocket card type to highlight types

5b5fb32

sarracini force-pushed the sarracini:bug_1432657 branch from 9bd7241 to 5b5fb32 Feb 23, 2018

sarracini removed the PR / Needs work label Feb 23, 2018

sarracini merged commit b033185 into mozilla:master Feb 23, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mardak mentioned this pull request Feb 28, 2018

Merge latest strings from activity-stream changeset f3e33da50e0bea2fad0a38a985c5eab1d98eb38a mozilla/activity-stream-l10n#11

Merged

as-pine-proxy added the pushed-to-pine label Mar 6, 2018

mozilla / activity-stream Archived

Fix Bug 1432657 - Add pocket card type to highlight types #3975

Fix Bug 1432657 - Add pocket card type to highlight types #3975

sarracini commented Feb 7, 2018 •

edited

Mardak left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sarracini commented Feb 14, 2018 •

edited

This comment has been minimized.

This comment has been minimized.

Mardak left a comment

sarracini commented Feb 20, 2018

Mardak commented Feb 20, 2018

This comment has been minimized.

This comment has been minimized.

sarracini commented Feb 22, 2018 •

edited

sarracini commented Feb 22, 2018

Mardak left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

as-pine-proxy commented Mar 6, 2018

mozilla / activity-stream Archived

Fix Bug 1432657 - Add pocket card type to highlight types #3975

Fix Bug 1432657 - Add pocket card type to highlight types #3975

Conversation

sarracini commented Feb 7, 2018 • edited

Mardak left a comment

This comment has been minimized.

Mardak Feb 13, 2018 Member

This comment has been minimized.

sarracini Feb 16, 2018 Author Contributor

This comment has been minimized.

Mardak Feb 13, 2018 Member

This comment has been minimized.

Mardak Feb 22, 2018 Member

This comment has been minimized.

Mardak Feb 13, 2018 Member

This comment has been minimized.

Mardak Feb 13, 2018 Member

This comment has been minimized.

sarracini Feb 14, 2018 Author Contributor

This comment has been minimized.

sarracini Feb 16, 2018 Author Contributor

This comment has been minimized.

Mardak Feb 13, 2018 Member

This comment has been minimized.

sarracini Feb 14, 2018 Author Contributor

This comment has been minimized.

piatra Feb 14, 2018 Collaborator

This comment has been minimized.

Mardak Feb 13, 2018 Member

This comment has been minimized.

Mardak Feb 13, 2018 Member

sarracini commented Feb 14, 2018 • edited

This comment has been minimized.

Mardak Feb 16, 2018 Member

This comment has been minimized.

sarracini Feb 16, 2018 Author Contributor

Mardak left a comment

sarracini commented Feb 20, 2018

Mardak commented Feb 20, 2018

This comment has been minimized.

sarracini Feb 21, 2018 Author Contributor

This comment has been minimized.

Mardak Feb 21, 2018 Member

sarracini commented Feb 22, 2018 • edited

sarracini commented Feb 22, 2018

Mardak left a comment

This comment has been minimized.

Mardak Feb 22, 2018 Member

This comment has been minimized.

Mardak Feb 22, 2018 Member

This comment has been minimized.

Mardak Feb 22, 2018 Member

This comment has been minimized.

sarracini Feb 23, 2018 Author Contributor

This comment has been minimized.

Mardak Feb 22, 2018 Member

This comment has been minimized.

Mardak Feb 22, 2018 Member

This comment has been minimized.

sarracini Feb 23, 2018 Author Contributor

as-pine-proxy commented Mar 6, 2018

sarracini commented Feb 7, 2018 •

edited

Mardak Feb 13, 2018
Member

sarracini Feb 16, 2018
Author Contributor

Mardak Feb 13, 2018
Member

Mardak Feb 22, 2018
Member

Mardak Feb 13, 2018
Member

Mardak Feb 13, 2018
Member

sarracini Feb 14, 2018
Author Contributor

sarracini Feb 16, 2018
Author Contributor

Mardak Feb 13, 2018
Member

sarracini Feb 14, 2018
Author Contributor

piatra Feb 14, 2018
Collaborator

Mardak Feb 13, 2018
Member

Mardak Feb 13, 2018
Member

sarracini commented Feb 14, 2018 •

edited

Mardak Feb 16, 2018
Member

sarracini Feb 16, 2018
Author Contributor

sarracini Feb 21, 2018
Author Contributor

Mardak Feb 21, 2018
Member

sarracini commented Feb 22, 2018 •

edited

Mardak Feb 22, 2018
Member

Mardak Feb 22, 2018
Member

Mardak Feb 22, 2018
Member

sarracini Feb 23, 2018
Author Contributor

Mardak Feb 22, 2018
Member

Mardak Feb 22, 2018
Member

sarracini Feb 23, 2018
Author Contributor