-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add usePersistentUserChoices hook to save device settings and username to local storage #683
Conversation
🦋 Changeset detectedLatest commit: 9bb9fb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
I like the idea, I'm wondering if the stuff that is in |
usePersistentUserChoices
instead of DeviceSettings
user-choices.ts
getLocalStorageObject function
/** | ||
* Whether to prevent saving user choices to local storage. | ||
*/ | ||
preventSave: boolean = false, |
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.
could we instead of the boolean parameter simply not call the function if the user doesn't intend to save?
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.
I wanted the default defaultUserChoices
(if no defaults are passed by the user) to be defined and handled within the core. This API evolved from that decision. We could move the same logic one layer up into components-react, but that would increase the API surface a bit, and (if we support more frameworks in the future) we would have to reimplement it.
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.
got it, that makes sense to me!
* Whether to prevent loading from local storage and return default values instead. | ||
* @defaultValue false | ||
*/ | ||
preventLoad: boolean = false, |
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.
same question as above
* Whether to prevent saving to persistent storage. | ||
* @defaultValue false | ||
*/ | ||
preventSave?: boolean; |
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.
wondering if save
and load
would make more sense than negating the option with prevent*
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.
I have the feeling that preventSave=true
tells you a bit more than just save=true
.
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.
What about skipSave: boolean
?
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.
Really just wanted to hear your opinion on it, I think your argument is fair. Let's stick with prevent*
then!
This pull request adds a new hook,
usePersistentUserChoices
, which provides functionality to save and load user choices. This will allow users to persist their device settings and username across sessions.The user choices object stored in local storage looks like this:
Settings in local storage are updated by changes in the
ControlBar
and thePreJoin
component. The general goal is to always use the media devices that were last used.I have added functionality to prevent the
usePersistentUserChoices
from writing to local storage, but by default we write to local storage.