-
Notifications
You must be signed in to change notification settings - Fork 35
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
Default settings #56
base: trunk
Are you sure you want to change the base?
Default settings #56
Conversation
<meta charset="utf-8"> | ||
<link rel="stylesheet" href="popup.css"> | ||
<meta charset="utf-8" /> | ||
<link rel="stylesheet" href="popup.css" /> |
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.
No reformatting please!
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.
oops i didn't realize i made this change it was the IDE i was using auto formatted it
@@ -6,7 +6,32 @@ const elementsList = document.getElementById('elements-list') | |||
const allElements = document.getElementById('all-elements') | |||
const indivElements = document.getElementById('individual-elements') | |||
const elementsTpl = document.getElementById('elements-tpl') | |||
|
|||
function storageAvailable(type) { |
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 checking is completely unnecessary, it's safe to assume that storing 4 numbers will never ever exceed the quota. (Also would testing window["localStorage"]
even work?! When the object is browser.storage.local
)
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.
that's a good point lol its not storing much i just saw it suggested to check for that. im not sure if window["localstorage"] would work first time really using JS.
Pretty sure what people want is not a button that would both save and restore (?) but rather a default being applied automatically, from a background script or directly from the content script if that works; not from the popup script. Also, there should be an option to only apply the settings per domain. Honestly it would be easier for me to do it myself than to guide someone towards a good implementation, but if you want a learning experience, I can go along with this a bit :) The first tip is respecting the existing code style of a project: no one likes PRs that randomly introduce formatting changes. |
so you mean i should created a separate script file for this functionality and have it so it runs in the background when someone starts the app. having it set by domain is a good idea ill try that too. thanks for the feedback |
i saw that someone had requested to be able to set default settings. i added this functionality. not experienced in extension development or JavaScript so i hope to get some criticism but it seems to be working for me. I added storage using the storage api to store settings so when you hit the set default button it takes the current settings and saves them in local storage then the next time the app is run it reads the storage values and sets them. when you click the reset button it clears the storage and works as it normally would. hope its good.