-
Notifications
You must be signed in to change notification settings - Fork 209
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
2657 create settingsjs file #2659
Changes from 7 commits
3146638
1b407bd
15eb24e
26ba520
fb2da9f
3929070
2fbcaf0
48e2658
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
class SettingsManager extends EventTarget { | ||
constructor() { | ||
super(); | ||
|
||
this.addEventListener('gfxSettingsChanged', e => { | ||
const settings = e.data; | ||
this.#setSettings('GfxSettings', settings); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to separate these events at all? Is there ever a case, besides the settings panel itself, where we are generally interested in a page of settings as opposed to a setting itself? What would be the purpose of such a notification/categorizing it this way for users of this API, other than confusion? IMO it makes more sense to emit that a specific setting value changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll change it to a single event. I also agree that a single value makes more sense. It's currently accepting the full set of perameters because of how the saveSettings functions are written in the settings tabs, But I'll adjust accordingly |
||
}); | ||
|
||
this.addEventListener('controlSettingsChanged', e => { | ||
const settings = e.data; | ||
this.#setSettings('ControlsSettings', settings); | ||
}); | ||
|
||
this.addEventListener('aiSettingsChanged', e => { | ||
const settings = e.data; | ||
this.#setSettings('AiSettings', settings); | ||
}); | ||
|
||
this.addEventListener('audioSettingsChanged', e => { | ||
const settings = e.data; | ||
this.#setSettings('AudioSettings', settings); | ||
}); | ||
|
||
|
||
} | ||
|
||
getSettings(key) { | ||
return localStorage.getItem(key); | ||
} | ||
|
||
getSettingsJson(key) { | ||
return JSON.parse(localStorage.getItem(key)); | ||
} | ||
|
||
#setSettings(key, settings) { | ||
localStorage.setItem(key, JSON.stringify(settings)); | ||
} | ||
} | ||
|
||
const settingsManager = new SettingsManager(); | ||
export default settingsManager; |
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 casing between event names and data is inconsistent. Also, we generally do not camelcase in event names, which is the standard for the web platform; it's usually all lowercase.
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.
noted