Add share feature #921

groovecoder · 2017-10-26T20:35:27Z

To spot-check ...

Install this version of XPI
Open 5 container tabs (will bump to 100 for release)
You should see a green "NEW" badge on the browserAction icon.
When you click it, you should see the "achievement panel" with links to rate, share, tweet, and done.

groovecoder · 2017-10-26T20:49:58Z

webextension/js/background/messageHandler.js

+    browser.storage.local.set({[key]: countOfContainerTabsOpened});
+
+    // When the user opens their _ tab, give them the achievement
+    if (countOfContainerTabsOpened === 5) {


NEED TO BUMP THIS TO 500 BEFORE MERGE!

groovecoder · 2017-10-26T20:50:21Z

webextension/js/popup.js

+    }
+  },
+
+  async setAchievementDone(achievementName) {


Need to figure out better logic here.

Updated the logic. This is ready to review and spot-check now, @jonathanKingston

So done means "read" essentially? It's a little confusing in name between getting the achievement and marking it done.

Essentially ... I put "Done" on the button because the user has to actually click it to clear the achievement. (Hopefully after they rate/tweet/share) So "Done" means whatever the user chooses to do: just read it, or read + rate, or read + share, etc.

I'll probably keep the name; setAchievementRead would imply that the user only has to see the achievement for it to go away.

TanviHacks · 2017-10-27T19:45:49Z

Lets make it 100 container tabs when this hits release users.

TanviHacks · 2017-10-27T19:47:05Z

Also, how are we counting? Any tab with a usercontextId!=0? There are many entrypoints for opening container tabs, so better to count the tabs than the entrypoints.

groovecoder · 2017-10-27T20:02:47Z

It's counting any container tab opened by any of the entry points when the user context is not firefox-default or firefox-private.

jonathanKingston · 2017-10-31T15:29:33Z

webextension/js/background/messageHandler.js

@@ -120,6 +128,29 @@ const messageHandler = {
    });
  },

+  async incrementCountOfContainerTabsOpened() {
+    const key = "containerTabsOpened";
+    const count = await browser.storage.local.get(key);


I think you can use get({[key]: 1}) right?

Instead of the presence checking below etc

jonathanKingston · 2017-10-31T15:56:09Z

webextension/js/background/messageHandler.js

+        // Don't count firefox-default, firefox-private, nor our own confirm page loads
+        if (tab.cookieStoreId !== "firefox-default" &&
+            tab.cookieStoreId !== "firefox-private" &&
+            tab.url.indexOf("confirm-page.html") === -1) {


Can we check instead for moz-extension protocol instead perhaps?

jonathanKingston · 2017-10-31T16:30:38Z

I left two comments which would be cool if we could fix those to be a little simpler. If you could add a comment explaining when achievements are gathered and "done" too just for future when we forget.

Other than that everything looks fine, I haven't tested at the moment though.

groovecoder · 2017-10-31T17:59:10Z

Updated. @SoftVision-EmilPasca - it's a tedious task, but could you test this branch that the achievement panel shows in the pop-up after opening 100 container tabs? (Easiest way is probably to assign a site to automatically open in a container, "Remember my decision", and then just open tabs to that site very quickly.)

SoftVision-EmilPasca · 2017-11-01T16:18:31Z

@groovecoder I have made a build from add-share-feature branch and tested on Windows 10 x64, Mac 10.12 and Ubuntu 14.04 with the latest Firefox release (56.0.2), the latest Nightly (58.0a1-20171030103605) and latest Beta(57.0b13-20171030163911).

I have tried different scenarios in which I've created 100 container tabs or more using combinations of custom and default containers, hidden container tabs, non-container tabs, using multiple windows, private windows, different sessions or disabling/enabling the add-on and restarting the browser.

During testing I've observed that the "achievement panel" is successfully shown regardless of the scenarios above if a number of 100 container tabs have been reached by the user.
Also the "Rate", "Share" and "Tweet" buttons correctly redirects to the desired pages and the "Done" button correctly closes the "achievement panel" when pressed.

If you think there are any more scenarios which I haven't covered, please let me know.

groovecoder · 2017-11-02T11:36:52Z

That's great, thanks @SoftVision-EmilPasca !

groovecoder · 2017-11-02T11:37:00Z

@jonathanKingston can I get an r+ here now?

jonathanKingston · 2017-11-02T14:18:04Z

Out of interest why are we using shortened urls for the sharing links?
Eg:
https://bit.ly/fb-share-mac-addon

Also these have a mac utm in their source?

If we are worried about length of HTML lines we can solve that with vars or something else
If we are worried about utm's being exposed when the user hovers over the URL, I would rather be genuine and show it all (I didn't actually test if hovering shows as it normally does)

... more to come end of train ride.

groovecoder · 2017-11-02T14:46:43Z

Yeah, I'm not super-happy with it. But the utm params didn't seem to work when I put them straight into the url. But I'll try a couple encoding tricks to avoid short urls if at all possible.

jonathanKingston · 2017-11-03T10:46:13Z

1.

Yeah, I'm not super-happy with it. But the utm params didn't seem to work when I put them straight into the url.

Hmm that is odd, anyway can we change the "mac" part, unless there is an explanation for it?
Perhaps it is the tweet text having spaces, you could pass it through the method we have that uses encodeUriComponent and some other changes (that covers weird edge cases that impact extensions)?

The code is in the assignManager, feel free to take the output and hard code it into the HTML

  encodeURLProperty(url) {
    return encodeURIComponent(url).replace(/[!'()*]/g, (c) => {
      const charCode = c.charCodeAt(0).toString(16);
      return `%${charCode}`;
    });
},

2.
Also we can now do async arrows: async () => { feel free to fix this or ignore (we can file a follow up to fix it all instead).

3.
Can you change tab.url.indexOf("moz-extension") === -1 with !tab.url.startsWith("moz-extension") as it prevents any edge case URLs that contain that in a param for some bizarre reason.

4.
Don't we have the following as a function call anyway?

+      browser.storage.local.set({achievements});
+      browser.browserAction.setBadgeBackgroundColor({color: "rgba(0,217,0,255)"});
+      browser.browserAction.setBadgeText({text: "NEW"});

5.
Are "webextension/img/fb-icon.png" and "webextension/img/twitter-icon.png" open source? Also is there a SVG version instead?

6.
const countOfContainerTabsOpened = count[key] += 1; to me implies you need to increment count too, which from what I can see you don't.

I would use: const countOfContainerTabsOpened = ++count[key];
or just reference count[key]:

count[key] += 1;
browser.storage.local.set({[key]: count[key]});

Other than that we are all good, feel free to merge after these 6.
None are blockers if you want to make them follow ups, although likely 1. utm code can't change after releasing to users.

groovecoder · 2017-11-06T15:11:06Z

The 'mac' part in this case means "Multi-Account Containers" - not Mac OS.
Let's do a follow-up enhancement for async arrow functions
fixed
There's a badge.displayBrowserActionBadge function, but it would need refactoring to be useful here and in popup.js. Probably another follow-up/fix-up?
Switched to CC-Attrib SVG icons
Updated/fixed.

TanviHacks · 2017-11-09T22:08:08Z

What happens if a user clicks "Tweet". Is the tweet content pre-populated for them to share or manually edit?

groovecoder · 2017-11-10T14:34:25Z

It's pre-populated but the user can change/update the text if they want.

jonathanKingston

LGTM :) if you could file those follow ups that would be great. Should be possible to mark them as good first bug too.

start containerTabsOpened counter

cb937df

groovecoder added the DO NOT MERGE label Oct 26, 2017

groovecoder force-pushed the add-share-feature branch from b25b7aa to c59e1af Compare October 26, 2017 20:49

groovecoder requested a review from jonathanKingston October 26, 2017 20:49

groovecoder commented Oct 26, 2017

View reviewed changes

groovecoder force-pushed the add-share-feature branch 3 times, most recently from 2662f9c to 6287bb1 Compare October 27, 2017 19:16

groovecoder force-pushed the add-share-feature branch from 6287bb1 to 2d6bccc Compare October 27, 2017 20:09

jonathanKingston reviewed Oct 31, 2017

View reviewed changes

groovecoder force-pushed the add-share-feature branch 2 times, most recently from e8f38ab to 6c59ac1 Compare October 31, 2017 16:16

groovecoder force-pushed the add-share-feature branch from 6c59ac1 to 659277f Compare October 31, 2017 17:57

groovecoder requested a review from SoftVision-EmilPasca October 31, 2017 17:59

groovecoder force-pushed the add-share-feature branch from 659277f to 15bf301 Compare November 6, 2017 15:11

when user reaches container tab count, show achievement panel

1e33637

groovecoder force-pushed the add-share-feature branch from 15bf301 to 1e33637 Compare November 6, 2017 15:12

jonathanKingston approved these changes Nov 14, 2017

View reviewed changes

groovecoder removed the DO NOT MERGE label Nov 16, 2017

This was referenced Nov 16, 2017

Refactor to async arrow functions #972

Closed

Refactor badge.displayBrowserActionBadge to be used in both popup.js and messageHandler.js #973

Open

jonathanKingston merged commit 3129814 into master Nov 16, 2017

groovecoder deleted the add-share-feature branch November 16, 2017 15:39

rrix mentioned this pull request Jan 20, 2018

Configuration to disable "achievements" #1085

Closed

groovecoder mentioned this pull request Dec 26, 2018

Refactor async arrow functions #1274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add share feature #921

Add share feature #921

groovecoder commented Oct 26, 2017 •

edited

groovecoder Oct 26, 2017

groovecoder Oct 26, 2017

groovecoder Oct 27, 2017

jonathanKingston Oct 31, 2017

groovecoder Oct 31, 2017

groovecoder Oct 31, 2017

TanviHacks commented Oct 27, 2017

TanviHacks commented Oct 27, 2017

groovecoder commented Oct 27, 2017

jonathanKingston Oct 31, 2017

jonathanKingston Oct 31, 2017

groovecoder Oct 31, 2017

jonathanKingston Oct 31, 2017

groovecoder Oct 31, 2017

jonathanKingston commented Oct 31, 2017

groovecoder commented Oct 31, 2017

SoftVision-EmilPasca commented Nov 1, 2017

groovecoder commented Nov 2, 2017

groovecoder commented Nov 2, 2017

jonathanKingston commented Nov 2, 2017

groovecoder commented Nov 2, 2017

jonathanKingston commented Nov 3, 2017

groovecoder commented Nov 6, 2017

TanviHacks commented Nov 9, 2017

groovecoder commented Nov 10, 2017

jonathanKingston left a comment

Add share feature #921

Add share feature #921

Conversation

groovecoder commented Oct 26, 2017 • edited

To spot-check ...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanviHacks commented Oct 27, 2017

TanviHacks commented Oct 27, 2017

groovecoder commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston commented Oct 31, 2017

groovecoder commented Oct 31, 2017

SoftVision-EmilPasca commented Nov 1, 2017

groovecoder commented Nov 2, 2017

groovecoder commented Nov 2, 2017

jonathanKingston commented Nov 2, 2017

groovecoder commented Nov 2, 2017

jonathanKingston commented Nov 3, 2017

groovecoder commented Nov 6, 2017

TanviHacks commented Nov 9, 2017

groovecoder commented Nov 10, 2017

jonathanKingston left a comment

Choose a reason for hiding this comment

groovecoder commented Oct 26, 2017 •

edited