feat(local-storage): store cvaa settings #45
Conversation
src/storage/storage-manager.js
Outdated
| * @returns {void} | ||
| */ | ||
| static setPlayerTextStyle(player: Player): void { | ||
| const textStyle = StorageManager.getStorage().textStyle; |
There was a problem hiding this comment.
Why get all the storage again? time consuming
call
StorageWrapper.getItem('textStyle')
There was a problem hiding this comment.
didn't know this API, 10x.
src/storage/storage-manager.js
Outdated
| static setPlayerTextStyle(player: Player): void { | ||
| const textStyle = StorageManager.getStorage().textStyle; | ||
| if (textStyle) { | ||
| player.textStyle = Utils.Object.mergeDeep(new TextStyle(), textStyle); |
There was a problem hiding this comment.
better to split to declaration and assignment
let textStyle = Utils.Object.mergeDeep(new TextStyle(), textStyle);
player.textStyle = textStyle;
There was a problem hiding this comment.
and on line 61 maybe change the name to textStyleObj
because this is not really the textStyle but the parsed object from the storage.
There was a problem hiding this comment.
regarding split:
let textStyle = Utils.Object.mergeDeep(new TextStyle(), textStyle);
is also declaration and assignment in one line...
regarding name: done.
src/storage/storage-manager.js
Outdated
| * @public | ||
| * @returns {void} | ||
| */ | ||
| static setPlayerTextStyle(player: Player): void { |
There was a problem hiding this comment.
why it gets the player reference again?
this is not already on the StorageManager ?
StorageManager._player
There was a problem hiding this comment.
you can't rely on the statics method invoking order. if i'm changing the order in setup.js:
setStorageTextStyle();
applyStorageSupport(kalturaPlayer);
it won't work.
i've remove the player ref.
applyStorageSupport gets it as param, why to storing it?
There was a problem hiding this comment.
Sounds legit, I was convinced
src/storage/storage-manager.js
Outdated
| StorageWrapper.setItem('muted', StorageManager._player.muted); | ||
| StorageWrapper.setItem('volume', StorageManager._player.volume); | ||
| player.addEventListener(player.Event.VOLUME_CHANGE, () => { | ||
| StorageWrapper.setItem('muted', player.muted); |
There was a problem hiding this comment.
@yairans
If u already touch this class, move please the muted to his own event (MUTE_CHANGE).
It's done this way in the past cause we didn't had mute event.
10x
There was a problem hiding this comment.
I'm not getting the separation of methods to apply config.
Maybe if the name config was changed to cachedSettings it made more clear sense what we are trying to do.
This feature is from kaltura player perspective so mute and textStyle are the same thing.
The difference is how they are applied to the playkit lib.
I would change the of setStorageConfig to setStorageSetting or maybe setSettingsFromStorage and in it do all storage based actions
src/storage/storage-manager.js
Outdated
| @@ -1,16 +1,17 @@ | |||
| // @flow | |||
| import StorageWrapper from './storage-wrapper' | |||
| import LoggerFactory from '../utils/logger' | |||
| import {Utils, TextStyle} from 'playkit-js' | |||
|
|
|||
| export default class StorageManager { | |||
| static StorageKeys = [ | |||
There was a problem hiding this comment.
Better move these to const keys as we are referring the strings in too many places already
src/storage/storage-manager.js
Outdated
| const VOLUME = 'volume'; | ||
| const AUDIO_LANG = 'audioLanguage'; | ||
| const TEXT_LANG = 'textLanguage'; | ||
| const TEXT_STYLE = 'textStyle'; |
There was a problem hiding this comment.
I don't like it like that
if we will have 20 keys, so we will create 20 consts?
(@yairans it was not said in anger :) )
I think we need to change StorageKeys to a key-value object:
static StorageKeys = {
KEY : "VALUE"
}
and when we will need to iterate the values we will simply do Object(StorageManager.StorageKeys).keys().forEach((storageKey...
Description of the Changes
CheckLists