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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #27 - Adds "Report site issue" option to the context menu for Firefox #30

Merged
merged 1 commit into from Sep 1, 2016

Conversation

Projects
None yet
2 participants
@akhilpandey95
Copy link
Member

commented Sep 1, 2016

Signed-off-by: AkhilHector akhilhector.1@gmail.com

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

Ref : #27

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

I think the code looks redundant, maybe i can declare a function which could be used for both context-menu and the web-extension. What say @miketaylr

something like this :
screenshot from 2016-09-01 22 38 45

PS : I don't like having redundant code

@@ -5,6 +5,26 @@
var prefix = 'https://webcompat.com/?open=1&url=';
var screenshotData = '';

chrome.contextMenus.create({
id: "webcompat",

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 1, 2016

Member

id is optional, so I'd recommend either leaving it out, or picking something more specific like webcompat-contextmenu.

This comment has been minimized.

Copy link
@akhilpandey95

akhilpandey95 Sep 1, 2016

Author Member

noted, then what about that redundant code ?

@@ -5,6 +5,26 @@
var prefix = 'https://webcompat.com/?open=1&url=';
var screenshotData = '';

chrome.contextMenus.create({
id: "webcompat",
title: "Report Website compatibility issue",

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 1, 2016

Member

This should probably be shorter since it's in a context menu (although I don't see a limit...). Maybe Report site issue? That's what the Fennec Nightly/Aurora menu option says is.

This comment has been minimized.

Copy link
@akhilpandey95

akhilpandey95 Sep 1, 2016

Author Member

Noted, anything else ?

contexts: ["all"]
});

chrome.contextMenus.onClicked.addListener(function (tab) {

This comment has been minimized.

Copy link
@miketaylr

miketaylr Sep 1, 2016

Member

Yeah, like you mentioned in your comment we can make this more DRY.

@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

A reportIssue method seems good. 👍

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

ok give me a second, let me ensure the mentioned changes are made

@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

@AkhilHector also, we want to make the same changes for the Opera and Chrome add-ons if you're interested in hacking on those as well.

@akhilpandey95

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

Sure, will do it @miketaylr

AkhilHector
Issue #27 - Adds webcompat report option to the context menu
Signed-off-by: AkhilHector <akhilhector.1@gmail.com>

@miketaylr miketaylr changed the title Issue #27 - Adds webcompat report option to the context menu Fixes #27 - Adds "Report site issue" option to the context menu for Firefox Sep 1, 2016

@miketaylr

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

r+, thanks @AkhilHector!

@miketaylr miketaylr merged commit f5a20a9 into webcompat:master Sep 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.