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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Context menu #763

Merged
merged 7 commits into from
Nov 18, 2016
Merged

Context menu #763

merged 7 commits into from
Nov 18, 2016

Conversation

sweeneyapps
Copy link
Contributor

@sweeneyapps sweeneyapps commented Nov 17, 2016

This is for issue #743, Context Menu under Rainforest Icon 馃憤

It will build context menu at startup and will make sure radio is checked on sub-menu from each option setting.

The context menu will set options for Confirm Work Assignment, Notification Sound, and Sound Repeat. After setting options on the context menu, it will make sure it's saved on chrome sync storage.

@shosti

@richardfuca
Copy link
Contributor

just wondering, why don't we also put the settings in the options page (the one in the extension settings)?

@sweeneyapps
Copy link
Contributor Author

sweeneyapps commented Nov 17, 2016

if we do the settings in the extension's options page then it will break the goal to keep up with rolling updates from the server because of the complication of having to host 2 files, one for options page and other for extension's main.js. It's easier to handle npm module by building it on the server and serving just one file, main.js to the extension.

@shosti
Copy link
Contributor

shosti commented Nov 17, 2016

@sweeneyapps looks like the menu items are duplicated for some reason:

screenshot

Copy link
Contributor

@shosti shosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good from a quick look, but I tried locally and it didn't appear to work (as well as having the double-menu issue). Any ideas what might be up?

@@ -154,6 +156,14 @@ export const mockChrome = (opts = {}) => {
setTimeout(() => callback(profileUserInfo));
},
};
const contextMenus = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a space above here for consistency?

const contextMenus = {
create: (option, callback) => {
createdMenus.push(option);
if (option.checked) checkedMenus.push(option);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use braces here (I always get nervous about brace-less if statements).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks.. now i'm working on it..

@@ -14,7 +14,6 @@ export const actions = deepFreeze({
CHECK_STATE: 'CHECK_STATE',
SET_PLUGIN_VERSION: 'SET_PLUGIN_VERSION',
SET_WORKER_PROFILE: 'SET_WORKER_PROFILE',
RATE_LIMIT_EXCEEDED: 'RATE_LIMIT_EXCEEDED',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 thanks for catching that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ur welcome

@sweeneyapps
Copy link
Contributor Author

right now, I'm hunting down the duplication bug.. I was able to reproduce the bug by reloading the plugin inside the console.

@sweeneyapps
Copy link
Contributor Author

I fixed the bug and I still have to write a test for it so when I get home later today, I'll work on it.

@sweeneyapps
Copy link
Contributor Author

bugfix done! :) Also with additional code to rebuild the menu w/ correct setting after tester set options on their profile page.

@majulink
Copy link

How can I get this context menu

Thanks

@sweeneyapps
Copy link
Contributor Author

sweeneyapps commented Nov 18, 2016

@majulink
Context menu is under code review and once it's approved, you will see it on the next extension update automatically.

Copy link
Contributor

@shosti shosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, and I tried it locally and it seems to work. I just have one request to prevent it from blowing up while people's plugins are updating.

});
};

const createContextMenu = (option) => {
Copy link
Contributor

@shosti shosti Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be a period between when the backend JS is loaded and people's plugins have updated when the extension won't have the contextMenus privilege. I checked and it looks like chrome.contextMenus is undefined in that case, so this will blow up. Can you add a check and maybe just return if chrome.contexteMenus is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.. will do.

@shosti shosti merged commit 6ce3971 into rainforestapp:develop Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants