-
Notifications
You must be signed in to change notification settings - Fork 11
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
Make extension icon open a tab on click #41
Conversation
added event handler for icon click to open new tab
must have action key for icon click event handler to work
originally console.log, now console.error
two unit tests for same function
removed redundant new line
added browser_action property
removed test for redundant url
Thanks for this contribution! Just a heads up, I'm OOO and won't get around to reviewing this for a week or two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great! Just a few small changes.
@@ -14,7 +14,8 @@ | |||
"background": { | |||
"service_worker": "ext-background.js" | |||
}, | |||
"action": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to each test-manifest.js
to simply verify this value. It would be helpful to comment in the test that an empty object activates the browser action icon so is different from an undefined action
value.
@@ -56,14 +56,19 @@ test('sets the post-uninstall URL', () => { | |||
.toHaveBeenCalledWith('https://tab.gladly.io/newtab/uninstalled/') | |||
}) | |||
|
|||
test('gracefully handles errors with setting the post-uninstall URL', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you accidentally replaced an existing test. Please re-add that test.
add post-uninstall url error handling
Manifest V2 uses browserAction key; V3 uses action key
browser_action property must exist for icon to be shown in toolbar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the contribution, @blue-putty!
Closes #17.
Changes proposed in this PR:
action
andbrowser_action
properties to respective Jest API mocks