-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix manifest v3 issue #13
base: master
Are you sure you want to change the base?
Conversation
Great, I was just working on this |
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.
Just a few suggestions, I will appreciate if you take a look!
"script.js", | ||
"style.css", | ||
"icon.png", | ||
"manifest.json", | ||
"csp.js" |
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.
Are you sure you need all these?
Only csp.js
is working for me.
script.src = chrome.runtime.getURL('public/csp.js') ; | ||
(document.head || document.documentElement).appendChild(script); |
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.
Refactor content.ts
Since csp.js
is a separate file, I think content.ts
is doing too many things.
We can remove this disabling business out of content.ts
and keep this as a content script instead of injecting.
csp.js
will inject the code instead, which can be something like this:
const disablingCode = ```$('#sortable_nav').sortable('destroy');
$('#sortable_nav').enableSelection();
```
const script = document.createElement('script');
script.textContent = disablingCode;
(document.head || document.documentElement).appendChild(script);
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.
This should be a separate pull request though.
"activeTab", | ||
"scripting" | ||
], | ||
"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.
The default_icon
key is recommended here (which is basically a copy-paste from the icons
above).
Also, you can add a default_title
here that is shown as a tooltip on hover:
"action": {
"default_icon": {
"32": "icon.png",
"48": "icon.png",
"64": "icon.png",
"128": "icon.png"
},
"default_title": "Click to run PS-Extender"
}
Manifest v2 is now deprecated, and thus the shift to v3 was needed