-
Notifications
You must be signed in to change notification settings - Fork 17
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
action menu - create settings page #52
Conversation
send event settings updated on load
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.
My suggestion is to review this PR commit-by-commit.
src/popup/src/App.vue
Outdated
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.
Created by npm create vue
and then I cleaned some files.
src/popup/src/App.vue
Outdated
}; | ||
}, | ||
mounted() { | ||
chrome.storage.local.get('showSnyk').then(({ showSnyk }) => (this.showSnyk = showSnyk)); |
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.
Using chrome.storage.local
. Most of the docs use chrome.storage.sync
so the settings will be synced with the account logged in another computer.
src/popup/src/App.vue
Outdated
chrome.storage.local | ||
.set({ showSnyk }) | ||
.then(() => chrome.storage.local.get()) | ||
.then(sendEventSettingsChanged); |
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 popup is running on the extension context, and to pass the settings to the injected script, we need to send a message to the content script, which can send a message
to the webpage (where the injected script is live).
src/custom-elements/store.js
Outdated
export const usePackageInfo = (type, name) => | ||
computed(() => { | ||
const packageInfo = store.packages[type]?.[name]; | ||
if (!packageInfo) return null; | ||
|
||
const filteredSources = Object.entries(packageInfo.sources).reduce((acc, [key, value]) => { | ||
if (settings[key] !== false && value) { | ||
acc[key] = value; | ||
} | ||
return acc; | ||
}, {}); | ||
|
||
return { ...packageInfo, sources: filteredSources }; | ||
}); |
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.
src/content.js
Outdated
@@ -27,6 +28,8 @@ export const mountContentScript = (contentScript) => { | |||
return; | |||
} | |||
|
|||
events.sendEventSettingsChangedToWebapp(); |
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.
Send the settings to the injected script after it is loaded.
DRAFT: not supported in Firefox, working on it: |
…nu---create-settings-page
src/custom-elements/store.js
Outdated
} | ||
if (!store.packages[type][name]) { | ||
store.packages[type][name] = { | ||
const packageStoreID = ({ type, name }) => `${type}$${name}`; |
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.
I prefer using /
as a separator in package ids instead of $
. Also, I think renaming the object packageID
-> package
const packageStoreID = ({ type, name }) => `${type}$${name}`; | |
const getPackageId = (package) => `${package.type}/${package.name}`; |
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.
I avoided using /
because it is an allowed character in packages like @scoped/package
.
I remember I saw a "package identifier convention" in one of the advisories, I will search for that.
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.
Can we use :
instead of $$
or /
?
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.
yes :
is also ok
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.
done
New devDevelopment@types/firefox-webext-browser
|
Co-authored-by: Jossef Harush Kadouri <[email protected]>
First work of #27.
In this PR I created the infrastructure for controlling the state of the extension from its popup.
See the review comments for specific implementation details.