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

Refactor options storage #1

Merged
merged 3 commits into from
Nov 6, 2018
Merged

Refactor options storage #1

merged 3 commits into from
Nov 6, 2018

Conversation

robacarp
Copy link
Owner

@robacarp robacarp commented Nov 6, 2018

No description provided.

@JoshAshby JoshAshby self-requested a review November 6, 2018 17:07
Copy link
Collaborator

@JoshAshby JoshAshby left a comment

Choose a reason for hiding this comment

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

hrrrrrrrrrrrm

mv web_extension/shared/version.js.dev web_extension/shared/version.js
}

trap restore_version EXIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

wat


cat << JS > web_extension/shared/version.js
// this file automatically updated at build time by bin/package
class Version { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

class Version {
  static number = "v$version"
}

?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Docs say static properties must declared outside the class. Because reasons.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

babel yo

Version.number = "v$version"
JS

pushd web_extension
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

I like to think of switching directories as a stack more than just cd -'ing

'use strict';

async function fetchImage() {
let feed_url = options.feed.url
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like a lot of these could be const'd

document.querySelector('info by-line').appendChild( link(item.author.url, item.author.name) )
document.querySelector('info venue').appendChild( link(item._meta.venue.url, item._meta.venue.name) )
document.querySelector('info name').appendChild(
Builder.link(item.external_url, item.content_text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm highly skeptical but also at this rate you'll have re-invented react or something soon 😂

Copy link
Owner Author

Choose a reason for hiding this comment

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

too heavyweight. Unfortunately the innerHTML = template thing doesn't work because it's not safe and things

let hideIfChecked = (checkboxes, toggled_control) => {
let visible = false
checkboxes = [].concat(checkboxes)
checkboxes.forEach(function(box_id) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

stylistically, you're mixing arrow functions with non-arrows ¯\_(ツ)_/¯

Copy link
Owner Author

Choose a reason for hiding this comment

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

a truth. I learned how to arrow things this morning...

}

async function read_storage(){
current_storage = await browser.storage.sync.get()
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is current_storage ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

just here..in the function...I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

ಠ_ಠ

}

async read () {
let promise = browser.storage.sync.get()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let promise = browser.storage.sync.get()
const stored_options = await browser.storage.sync.get()
[...]
return stored_options

but also is it .sync if its returning a promise?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I did it this way because I want to return the promise up the chain, so I can await it elsewhere. I had a reason.

Copy link
Owner Author

Choose a reason for hiding this comment

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

And the deceptively named .sync is for configuration which is sync'd by the firefox browser

this.clock_persistence = "Subtle"
this.info_persistence = "Subtle"
this.clock_flash = false
this.date_format = "good"
Copy link
Collaborator

Choose a reason for hiding this comment

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

<_<

class FeedOptions extends OptionsSubset {
constructor () {
super()
this.url = "https://robacarp.github.io/photographic_start/feed.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make this an option too! 🎉 🐠

Copy link
Owner Author

Choose a reason for hiding this comment

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

#someday

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well technically, it is an option now. You just have to devtools it to change it. But whatever is stored in the browser.sync storage will prevail.

@robacarp robacarp merged commit 8a9ced8 into master Nov 6, 2018
@robacarp robacarp deleted the refactor_options branch November 6, 2018 21:40
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