Added extension badges #7050

jonathansampson · 2017-02-04T00:30:22Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Fixes #5366
Fixes #5367

Test Plan:

Enable the LastPass extension
From chrome-extension://hdokiejnpimakedhajhdlcegeplioahd/tabDialog.html?dialog=createAccountSimple, open Developer Tools (Ctrl+Shift+I)
From console, run chrome.browserAction.setBadgeText({text:"27"})
From console, run chrome.browserAction.setBadgeBackgroundColor({color:'#FF0000'})

bbondy · 2017-02-05T02:00:09Z

Super pumped you did this great work but I have to bump to 0.13.3 because we're past frozen date, sorry about that.

jonathansampson · 2017-02-05T02:55:50Z

@bbondy No worries; with that added time, I'll do a bit of polishing.

bbondy · 2017-02-05T03:53:19Z

cool thanks, 0.13.3 freeze is Monday end of day, but historically we're bad about respecting the freeze dates :)

jonathansampson · 2017-02-05T04:01:53Z

@bbondy I'll have my changes in before then :)

jonathansampson · 2017-02-06T20:52:57Z

@bradleyrichter I put in some logic to center wider badges. Presently, this means anything wider than 20 pixels. Please feel free to polish up any of the styles.

bradleyrichter · 2017-02-07T21:37:29Z

@jonathansampson Ok, I found a tiny size that is still legible at 72dpi.

5pt Arial

padding: 1px/2px

Your centering plan should work fine.

When we apply this to the brave logo, we should use a dark grey BG color:

bsclifton · 2017-02-10T17:26:00Z

@jonathansampson I don't recall seeing an issue for this (cc: @bradleyrichter) but we have an Asana issue captured for a similar badge on the Lion icon too (so that you know how many items blocked). Would you be able to work on that too?

bridiver · 2017-02-10T18:13:46Z

Badges? We don't need no stinkin' badges!

bridiver · 2017-02-10T18:15:22Z

++ as long as this meets @bradleyrichter's ui specs on all platforms

alexwykoff · 2017-02-21T23:10:55Z

Badges are happening!

Didn't notice any for 1pass even though I had logins for the sites, but may be due to the signing thing for local runs vs builds.

My only critique is that I find the value hard to read, but I do recognize that something is there.

My only negative mark is that the pocket icon seems to have regressed.

Will follow up in a moment with test run results from merging latest master. No merge conflicts detected.

alexwykoff · 2017-02-21T23:12:55Z

1179 tests passed, 39 pending, 6 failed.
All 6 were timeouts and appeared to be of our intermittent variety.

👍 for merge pending review of pocket icon regression ^^

bridiver · 2017-02-21T23:25:42Z

@alexwykoff I don't think 1pw has badges. I've never seen any on Chrome or Safari

bsclifton · 2017-02-23T08:28:23Z

@alexwykoff are you sure the icon for Pocket regressed? When I run master on Windows (without this patch), I'm seeing that icon too. My guess is that the extension has a new icon (maybe Windows only?) It should be the same in Chrome on Windows

cc: @srirambv

srirambv · 2017-02-23T10:51:35Z

@bsclifton I see it on preview 2 but only on x64 but not ia32 build.

srirambv · 2017-02-23T12:27:52Z

It seems to be only in Preview 2. Preview 1 has the same icon on both x64 & ia32. Problem with this icon is there is no visual confirmation if the page is saved or not.

bsclifton · 2017-02-23T17:54:30Z

@jonathansampson could you squash this up and add some testing steps? @alexwykoff has already reviewed, I can check it out too and if 👍 we can merge 0.13.6

bsclifton · 2017-03-13T22:43:39Z

Rebased and squashed; @NejcZdovc, I added you as a review for when you have time 😄

@jonathansampson would you be able to edit the original post here to provide a test plan? Looks like we'll want to manually install Honey and then do something which causes updates to happen. More info is appreciated 😄

jonathansampson · 2017-03-14T07:21:05Z

@bsclifton I can edit, and add a test plan. What relevance is the update action? To test this, we'd want to install an extension, and confirm that the browser action icon is one from the manifest.browser_action.icons property (whether it be a string, or object).

bsclifton · 2017-03-14T07:45:48Z

@jonathansampson sorry- I just meant update as in something that causes the numbers to show inside the icon (like in your screenshot at the top, LastPass has the number 27 in it).

I guess that is a good test- just pick LastPass as the default password manager and add some passwords 😄

NejcZdovc

Please use aphrodite and not less

NejcZdovc · 2017-03-14T08:08:31Z

app/renderer/components/browserAction.js

+        className={cx({
+          extensionButton: true
+        })}
+        inlineStyles={{


Let's switch to aphrodite

NejcZdovc · 2017-03-14T08:08:42Z

app/renderer/components/browserActionBadge.js

+  render () {
+    return <div
+      ref='badge'
+      className='browserActionBadge'


Let's use aphrodite

NejcZdovc · 2017-03-14T08:08:56Z

less/navigationBar.less

@@ -511,11 +511,37 @@
    flex-direction: row;
    margin-left: 3px;

+    .browserActionButton {


No more less 😃

bsclifton · 2017-03-14T15:28:44Z

@jonathansampson we can help you with the Aphrodite refactor (in fact, @cezaraugusto had put together a really sweet guide which you might find helpful 😄 )

In the meantime, I'll move this to 0.13.7 😄

NejcZdovc · 2017-03-14T17:17:10Z

@bsclifton @jonathansampson yeah I can help you with it, not a problem

jonathansampson · 2017-03-14T21:03:26Z

@bsclifton Sorry for the Testing Plan confusion. Half-way through this thread became about a Pocket icon regression. That threw me off.

Test Plan is now added. Essentially, call the two primary APIs from a LastPass extension resource. When enabled, LastPass opens up a chrome-extension: resource for the purposes of creating an account, or logging in. This page will suffice (as far as access/privileges go) to update the LastPass badge with color and text.

Working on Aphrodite changes now.

jonathansampson · 2017-03-16T18:24:21Z

@NejcZdovc Moved to Aphrodite (as much as I could). There are a couple of places where we don't know the value of the style property until runtime. Let me know if there's anything that could be further improved.

jonathansampson · 2017-03-21T02:50:00Z

@NejcZdovc @bridiver @bradleyrichter Can I get a fresh review? :)

NejcZdovc

LGTM

NejcZdovc · 2017-03-23T19:25:57Z

app/renderer/components/browserActionBadge.js

+const styles = StyleSheet.create({
+  browserActionBadge: {
+    color: 'white',
+    bordeRadius: '2px',


Typo: bordeRadius -> borderRadius

NejcZdovc · 2017-03-23T19:40:39Z

app/renderer/components/browserActionBadge.js

+
+const styles = StyleSheet.create({
+  browserActionBadge: {
+    color: 'white',


Please use hex #FFF

bridiver · 2017-03-23T20:04:31Z

can we get some tests for this? Preferably with a mock extension that is only enabled for testing

- Centers wide badges - Migrate to Aphrodite - Use Hex Colors

jonathansampson · 2017-03-24T20:22:27Z

@bridiver I believe that's doable.

bsclifton

Great job on this! 😄 It hasn't been re-reviewed since @NejcZdovc had given the feedback (which you then implemented). I followed the test steps and verified this works / looks good. ++++!

jonathansampson added design A design change, especially one which needs input from the design team. feature/extensions labels Feb 4, 2017

jonathansampson added this to the 0.13.2 milestone Feb 4, 2017

jonathansampson requested review from bridiver and bradleyrichter February 4, 2017 00:30

bbondy modified the milestones: 0.13.3, 0.13.2 Feb 5, 2017

luixxiul mentioned this pull request Feb 6, 2017

Auto include button is too small #7087

Closed

bsclifton assigned jonathansampson Feb 23, 2017

bsclifton modified the milestones: 0.13.6, 0.13.5 Feb 23, 2017

bsclifton requested a review from NejcZdovc March 13, 2017 22:41

bsclifton force-pushed the badge-text branch from d981e74 to 9da4b6c Compare March 13, 2017 22:43

NejcZdovc suggested changes Mar 14, 2017

View reviewed changes

bsclifton modified the milestones: 0.13.7, 0.13.6 Mar 14, 2017

bsclifton force-pushed the badge-text branch 2 times, most recently from d11a1ee to cc8d9ef Compare March 16, 2017 18:07

NejcZdovc approved these changes Mar 21, 2017

View reviewed changes

NejcZdovc suggested changes Mar 23, 2017

View reviewed changes

NejcZdovc reviewed Mar 23, 2017

View reviewed changes

Adds support for badges

cec76c4

- Centers wide badges - Migrate to Aphrodite - Use Hex Colors

jonathansampson force-pushed the badge-text branch from e52306f to cec76c4 Compare March 24, 2017 20:10

bsclifton self-requested a review April 3, 2017 06:13

bsclifton approved these changes Apr 3, 2017

View reviewed changes

bsclifton added QA/test-plan-specified release-notes/include labels Apr 3, 2017

bsclifton merged commit bd8eb8b into master Apr 3, 2017

bsclifton deleted the badge-text branch April 3, 2017 06:39

This was referenced Apr 4, 2017

Extension API: chrome.browserAction.setBadgeText #5366

Closed

Extension API: chrome.browserAction.setBadgeBackgroundColor #5367

Closed

alexwykoff changed the title ~~Adds support for badges~~ Added extension badges Apr 13, 2017

Added extension badges #7050

Added extension badges #7050

Conversation

jonathansampson commented Feb 4, 2017 • edited by bsclifton Loading

Test Plan:

bbondy commented Feb 5, 2017

jonathansampson commented Feb 5, 2017

bbondy commented Feb 5, 2017

jonathansampson commented Feb 5, 2017

jonathansampson commented Feb 6, 2017

bradleyrichter commented Feb 7, 2017

bsclifton commented Feb 10, 2017

bridiver commented Feb 10, 2017

bridiver commented Feb 10, 2017

alexwykoff commented Feb 21, 2017

alexwykoff commented Feb 21, 2017

bridiver commented Feb 21, 2017

bsclifton commented Feb 23, 2017

srirambv commented Feb 23, 2017

srirambv commented Feb 23, 2017 • edited Loading

bsclifton commented Feb 23, 2017 • edited Loading

bsclifton commented Mar 13, 2017 • edited Loading

jonathansampson commented Mar 14, 2017

bsclifton commented Mar 14, 2017

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc Mar 14, 2017

Choose a reason for hiding this comment

NejcZdovc Mar 14, 2017

Choose a reason for hiding this comment

NejcZdovc Mar 14, 2017

Choose a reason for hiding this comment

bsclifton commented Mar 14, 2017

NejcZdovc commented Mar 14, 2017

jonathansampson commented Mar 14, 2017 • edited Loading

jonathansampson commented Mar 16, 2017

jonathansampson commented Mar 21, 2017

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc Mar 23, 2017

Choose a reason for hiding this comment

jonathansampson Mar 24, 2017

Choose a reason for hiding this comment

NejcZdovc Mar 23, 2017

Choose a reason for hiding this comment

jonathansampson Mar 24, 2017

Choose a reason for hiding this comment

bridiver commented Mar 23, 2017

jonathansampson commented Mar 24, 2017

bsclifton left a comment

Choose a reason for hiding this comment

jonathansampson commented Feb 4, 2017 •

edited by bsclifton

Loading

srirambv commented Feb 23, 2017 •

edited

Loading

bsclifton commented Feb 23, 2017 •

edited

Loading

bsclifton commented Mar 13, 2017 •

edited

Loading

jonathansampson commented Mar 14, 2017 •

edited

Loading