Skip to content

Default settings#56

Open
tikitiki2 wants to merge 4 commits intovalpackett:trunkfrom
tikitiki2:trunk
Open

Default settings#56
tikitiki2 wants to merge 4 commits intovalpackett:trunkfrom
tikitiki2:trunk

Conversation

@tikitiki2
Copy link
Copy Markdown

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.

Comment thread popup.html
<meta charset="utf-8">
<link rel="stylesheet" href="popup.css">
<meta charset="utf-8" />
<link rel="stylesheet" href="popup.css" />
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread popup.js
const indivElements = document.getElementById('individual-elements')
const elementsTpl = document.getElementById('elements-tpl')

function storageAvailable(type) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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