Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1527500 - Save to pocket and bookmark menu options #4863

Merged
merged 2 commits into from Apr 1, 2019

Conversation

@punamdahiya
Copy link
Collaborator

commented Mar 21, 2019

No description provided.

@punamdahiya punamdahiya force-pushed the punamdahiya:bug-1527500 branch from 1167fb2 to fd3bcf2 Mar 21, 2019

if (item.url === action.data.url) {
const {bookmarkGuid, bookmarkTitle, dateAdded} = action.data;
return Object.assign({}, item, {
bookmarkGuid,

This comment has been minimized.

Copy link
@Mardak

Mardak Mar 22, 2019

Member

Just a quick glance.. this is the only place bookmarkGuid is set, i.e., when the bookmark is added? This won't persist after restarting firefox or when the feed refreshes as that would probably want DiscoveryStreamFeed changes to query every single url, but this loss-on-restart/refresh behavior might be acceptable for the initial implementation and have a bug for later/backlog ?

This comment has been minimized.

Copy link
@punamdahiya

punamdahiya Mar 22, 2019

Author Collaborator

we have https://bugzilla.mozilla.org/show_bug.cgi?id=1531596 for items losing bookmarked state on restart.

accumulator[feed_url] = {
data: {
...state.feeds.data[feed_url].data,
recommendations: state.feeds.data[feed_url].data.recommendations.filter(r => r.url !== action.data.url),

This comment has been minimized.

Copy link
@Mardak

Mardak Mar 22, 2019

Member

I didn't look that closely at all the others, but it seems like a higher order function that takes a function returning new recommendations might clean this up:

case BLOCKED:
  return HOF(recs => recs.filter(r => …));
case SAVED:
  return HOF(recs => recs.map(item => …));

This comment has been minimized.

Copy link
@Mardak

Mardak Mar 27, 2019

Member

Here's what I was thinking from our chat earlier (totally untested! mostly just copy pasted stuff 😉 ):

function DiscoveryStream(prevState, action) {
  const updateLinks = onLinks => {
      // Return if action data is empty, or spocs or feeds data is not loaded
      return !action.data || !prevState.spocs.loaded || !prevState.feeds.loaded ? prevState : {
        ...prevState,
        spocs: {
          ...prevState.spocs,
          data: prevState.spocs.data.spocs ? {spocs: onLinks(prevState.spocs.data.spocs)} : {},
        },
        feeds: {
          ...prevState.feeds,
          data: Object.keys(prevState.feeds.data).reduce((accumulator, feed_url) => {
            accumulator[feed_url] = {
              data: {
                ...prevState.feeds.data[feed_url].data,
                recommendations: onLinks(prevState.feeds.data[feed_url].data.recommendations),
              },
            };
            return accumulator;
          }, {});
        },
      };
  };

  switch(action.type) {
    case at.PLACES_LINK_BLOCKED:
      return updateLinks(recs => recs.filter(r => r.url !== action.data.url));
  }
}

This is assuming the same onLinks callback can be used for feed links and spocs.

@punamdahiya punamdahiya force-pushed the punamdahiya:bug-1527500 branch 2 times, most recently from 7c704cf to 1c6fc63 Mar 26, 2019

bookmarkTitle: data.title,
url,
},
});

This comment has been minimized.

Copy link
@punamdahiya

punamdahiya Mar 27, 2019

Author Collaborator

@Mardak Took a shot at checking first if bookmark exists before calling addBookmark on NewtabUtils. As per places team, this check shouldn't be expensive call. On rebookmark, story shows 'Remove bookmark' state on page refresh, trying to figure why?

This comment has been minimized.

Copy link
@Mardak

Mardak Mar 27, 2019

Member

Just noting from our chat, it doesn't update existing pages as we need to Broadcast the action. However, if we end up in the situation where we incorrectly show "Bookmark" action we'll handle that in https://bugzilla.mozilla.org/show_bug.cgi?id=1531596 so probably good to update that bug title and add your findings.

This comment has been minimized.

@punamdahiya punamdahiya force-pushed the punamdahiya:bug-1527500 branch from 1c6fc63 to 89d23f4 Mar 28, 2019

@punamdahiya punamdahiya force-pushed the punamdahiya:bug-1527500 branch from 89d23f4 to 76b396d Mar 28, 2019

@punamdahiya punamdahiya requested a review from ScottDowne Mar 28, 2019

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2019

@Mardak looks like we should add link to place utils history when bookmark is added https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/SectionsManager.jsm#445

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2019

@ncloudioj Including you to help review telemetry part of the PR essentially helping validate required telemetry from save to pocket and bookmark menu options in pocket new tab is working similar to activity stream. Thanks!

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2019

@Mardak looks like we should add link to place utils history when bookmark is added https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/SectionsManager.jsm#445

Please ignore, verified with Wolasi as not needed, this part of code is related to highlight and dedupe logic.

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2019

@ScottDowne @Mardak PR looks good to review, the only issue open is https://bugzilla.mozilla.org/show_bug.cgi?id=1539904 to be fixed in its own PR. Thanks

@ncloudioj

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@punamdahiya The telemetry bit looks good, verified in the local build.

r=nanj 🚢

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2019

Updated with delete and archive icons received from Wolasi

Screenshot 2019-03-28 18 16 57

@gvn gvn requested review from gvn and removed request for ScottDowne Apr 1, 2019

@gvn

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

Just flagging that Save to Pocket works, but I see no feedback in UI after I click it.

Is that expected?

Looks like current experience is that "Saved to Pocket" appears in the footer of the card (pretty subtle IMO):

Screen Shot 2019-04-01 at 11 27 30 AM

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 1, 2019

Just flagging that Save to Pocket works, but I see no feedback in UI after I click it.

Is that expected?

Yes that's out of scope and expected

Looks like current experience is that "Saved to Pocket" appears in the footer of the card (pretty subtle IMO):

@gvn

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

Ok, aside from that looks good and works well!

If we don't have a ticket to add UI feedback on save we probably should file one in the interest of keeping parity between DS and AS.

@gvn

gvn approved these changes Apr 1, 2019

@punamdahiya

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 1, 2019

@punamdahiya punamdahiya merged commit c23163b into mozilla:master Apr 1, 2019

2 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -0,0 +1 @@
<svg width="16" height="16" xmlns="http://www.w3.org/2000/svg"><path d="M1 14.667V4h14v10.667c0 .736-.597 1.333-1.333 1.333H2.333A1.333 1.333 0 0 1 1 14.667zM1 0h14a1 1 0 0 1 0 2H1a1 1 0 1 1 0-2zm9.341 7.247l-3.295 2.884-.839-.838a1 1 0 1 0-1.414 1.414l1.5 1.5a1 1 0 0 0 1.366.046l4-3.5a1 1 0 0 0-1.318-1.506z" fill="#000" fill-rule="evenodd"/></svg>

This comment has been minimized.

Copy link
@Mardak

Mardak Apr 3, 2019

Member

The rest of the svgs use context fill instead of hardcoded values, so this will lead to unexpected behaviors:
Screen Shot 2019-04-03 at 11 56 03 AM

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1541543

This comment has been minimized.

Copy link
@punamdahiya

punamdahiya Apr 3, 2019

Author Collaborator

Good catch! Submitted PR with the fix #4882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.