Skip to content

Conversation

@JoshTheDerf
Copy link
Collaborator

Should've used multiple commits, my bad.

Everything appears to be working on all browsers listed, but there's likely a performance hit due to Firefox still having trouble with specified arguments to browser.storage.local.get and therefore having to use null to fetch everything.

The build script is not bulletproof in any way, shape, or form.

Discussion pull request, not for direct merging without changes.

@Nateowami
Copy link
Owner

Wow, this is great, thanks!

Seems there's a lot of code in /src. I wonder if we could slim it down (not that that relates to this PR).

This looks pretty good (still have to try it out). Perhaps we should make functions get and set that call chrome.storage.local.get and chrome.storage.local.set.

function get(key, fn) {
  if(chrome) {
    chrome.storage.local.get(key, fn);
  }
  else {
    browser.storage.local.get(null, fn);
  }
}

This would shorten code a little, and remove the possible performance issue except from Firefox. It would also allow us to change things more easily when Firefox fixes the bug that requires us to use null. Seems that would be more maintainable.

On the subject of code cleanup, would could probably cut the size of the code by a lot if we used Ractive and Foundation... just a thought.

I'll try to get back to this again a little later.

@JoshTheDerf
Copy link
Collaborator Author

Yes, using a global get/set function would probably be useful, at least in the main file, though it would have to be implemented differently.

And yeah, using Ractive/Foundation or Bootstrap would probably not be a bad idea, but I'm too lazy to do so at the moment. (Actually don't need Backloader at the moment here in the U.S.)

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