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

Default settings #56

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Default settings #56

wants to merge 4 commits into from

Conversation

tikitiki2
Copy link

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.

<meta charset="utf-8">
<link rel="stylesheet" href="popup.css">
<meta charset="utf-8" />
<link rel="stylesheet" href="popup.css" />
Copy link
Owner

Choose a reason for hiding this comment

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

No reformatting please!

Copy link
Author

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) {
Copy link
Owner

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)

Copy link
Author

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.

@valpackett
Copy link
Owner

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.

@tikitiki2
Copy link
Author

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

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.

2 participants