2657 create settingsjs file #2659
Conversation
settings-manager.js
Outdated
| constructor() { | ||
| super(); | ||
|
|
||
| this.addEventListener('gfxSettingsChanged', e => { |
There was a problem hiding this comment.
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.
settings-manager.js
Outdated
|
|
||
| this.addEventListener('gfxSettingsChanged', e => { | ||
| const settings = e.data; | ||
| this.#setSettings('GfxSettings', settings); |
There was a problem hiding this comment.
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.
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
|
Doesn't look like this will make it in. |
This PR adds and event driven settings manager to handle getting/setting user settings to local storage.