Adds support for Honey #8120
Adds support for Honey #8120
Conversation
js/about/preferences.js
Outdated
| }, | ||
| honey: { | ||
| extensionName: 'Honey' | ||
| } | ||
| } |
There was a problem hiding this comment.
@bsclifton Looking at this a second time, should we instead go with something like this?
['Pocket', 'Honey'].reduce( (curr, key) => {
return ( curr[ key.toLowerCase() ] = { extensionName: key } ), curr
}, {})There was a problem hiding this comment.
@jonathansampson while that is pretty awesome (I love reduce!), I think the above is more readable
There was a problem hiding this comment.
Muon 2.57.9 has been released so I gave this a try (after blowing away node_modules and redoing npm install).
The extension for me is always being grayed out. It's not clickable. Possibly related, I am seeing these errors logged (from standard out, ex: when launching Brave via the command line)
[24868:775:0410/000140.848449:ERROR:extension_function_dispatcher.cc(613)] Unknown Extension API - cookies.getAll
I can relaunch Brave (with Honey still enabled) and it doesn't make a difference
|
@bsclifton I'll investigate. Thanks! |
|
any update on this? |
|
Looks like #6262 is needed. |
jonathansampson
force-pushed
the
ext_honey
branch
2 times, most recently
from
562b1f3
to
2ba0492
Compare
|
@bsclifton I've identified and fixed the problem of icons not changing. This PR now resolves #8500, #8501, and #8502. The one remaining issue is captured by #6262, which @kevinlawler is presently working to deliver. |
There was a problem hiding this comment.
pls also add image to app/extensions/brave/img/extensions and to the dummy data inside extensionUtil.js, and also update locale with the same pattern as others, so it could be visible for users while not installed.
I recall you asked me to but didn't include as it wasn't merged yet. I can do that btw if you prefer
jonathansampson
force-pushed
the
ext_honey
branch
from
8aa3ef5
to
9e0a041
Compare
|
@cezaraugusto Done, I believe :) |
jonathansampson
force-pushed
the
ext_honey
branch
from
0f93d3c
to
af5dcc4
Compare
| let tabAction = browserAction.getIn(['tabs', activeTabId.toString()]) | ||
| if (tabAction) { | ||
| tabAction = tabAction.set('title', browserAction.get('title')) | ||
| tabAction = tabAction.set('base_path', browserAction.get('base_path')) |
There was a problem hiding this comment.
@bridiver The tab-specific action object lacks a title and base_path. I'm plucking them off of the more general browser action object. Is this the right way to resolve their absence, or should we be adding them when the object is passing through Muon?
| @@ -29,3 +29,4 @@ torrent= Torrent Viewer | |||
| torrentDesc=Uses WebTorrent to display torrents directly in the browser. Supports torrent files and magnet links. | |||
| vimium= Vimium | |||
| vimiumDesc= | |||
| honeyDesc=Automatically find and apply coupon codes when you shop online! | |||
There was a problem hiding this comment.
you need a name here as well, like honey=Honey. If the extension has a different name like pocket (which shows as "Save to Pocket"), you'll need to use the same first letter for the variable as well, otherwise, we can't sort by name in extensions table. (just fyi, I don't think this applies here).
otherwise everything ok inside extensionsTab ++
There was a problem hiding this comment.
@bsclifton honey=Honey was originally suggested here. @cezaraugusto, please see the discussion here regarding this change.
|
@cezaraugusto That string should be taken care of now. |
|
Great, thanks @jonathansampson 😄 will review soon |
There was a problem hiding this comment.
Getting a crash when starting
I made sure to destroy node_modules and redo npm install. I rebased the PR locally also, just to make sure. Muon version is 2.58.8.
The error which I'm seeing logged to console over and over is:
[48704:775:0501/154112.639790:ERROR:extension_function_dispatcher.cc(585)] Unknown Extension API - cookies.getAll
Brave just exits and does not present the "report an error" dialog. I don't see an associated crash report in stats.brave.com either
| @@ -29,3 +29,5 @@ torrent= Torrent Viewer | |||
| torrentDesc=Uses WebTorrent to display torrents directly in the browser. Supports torrent files and magnet links. | |||
| vimium= Vimium | |||
| vimiumDesc= | |||
| honey=Honey | |||
There was a problem hiding this comment.
I don't think this is needed, but it doesn't hurt anything... Since the extension is a product name, it shouldn't be translated. But... using Japanese as an example... I could see writing this word using Katakana more helpful to the user. cc: @luixxiul
There was a problem hiding this comment.
@bsclifton The in-progress fix for cookies.getAll is here:
https://github.com/brave/muon/compare/chrome-cookies-getall
This should either suppress that error outright or should be able to be modified in some way to do so. I think it is waiting on further testing.
I'm on Slack if anyone with an interest in this wants to discuss.
There was a problem hiding this comment.
it shouldn't be translated. But... using Japanese as an example... I could see writing this word using Katakana more helpful to the user. cc: @luixxiul
Still in Japan people do not call "Pocket" as ポケット. I remember the issue happened on Chinese l10n before. / CC: @alexwykoff
For me, we could safely hardcode the name of every extention/product developed in English, as FF/Chrome do. People do not translate Gmail right?
There was a problem hiding this comment.
@kevinlawler We should probably open up a PR to merge your cookies.getAll stub into Muon. Thoughts? cc @bsclifton
There was a problem hiding this comment.
I think the convo happening regarding extensions name is my fault. I actually created that for extensions such as LastPass where the name is LastPass: Free Password Manager -- which includes product name + tagline. If that's desirable to have the name as-is I +1 on hardcoding names. Sorry for confusion :)
jonathansampson
force-pushed
the
ext_honey
branch
from
8e3edef
to
8085f63
Compare
|
@bsclifton @bbondy Conflicts resolved. |
|
IMO the badge needs more contrast to read the count easily. How about changing it into black like this (unless the green is the theme color)?
CC @bradleyrichter for his insight on that. |
|
@luixxiul The colors are chosen by the extension author, not by the browser. |
|
@jonathansampson even with a fresh session, I'm always seeing the badge as grayed out (I also tried restarting after enabling it). However, it works great |
There was a problem hiding this comment.
Looking into the Muon dev tools, I inspected the image and it's actually gray in the extension itself:
file:///Users/clifton/Library/Application%20Support/brave-development/Extensions/bmnlcjabgnpnenekpadlanbbkooimhnj/9.7.7/icons/default-19.png
file:///Users/clifton/Library/Application%20Support/brave-development/Extensions/bmnlcjabgnpnenekpadlanbbkooimhnj/9.7.7/icons/default-38.png
Since the functionality itself works, I think we're good 😄 👍
|
@jonathansampson I'll leave the PR open (see above comments). If that is OK default behavior, this is ready for you to merge! I wouldn't be surprised if it's gray until you login (I didn't test logging in- only the behavior which didn't work before). |
|
Found out what I was missing! You have to be visiting a site which has deals in order for the icon to light up. This is expected behavior that I was not aware of. Looks great! 😄 |
Test Plan (NOTE: requires Muon > 2.57.8)
Automated test plan
npm run test -- --grep="Honey installs when preference is enabled"Description
git rebase -ito squash commits (if needed).Fixes #8118
Fixes #8500
Fixes #8501
Fixes #8502