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

Add usePersistentUserChoices hook to save device settings and username to local storage #683

Merged
merged 19 commits into from
Nov 6, 2023

Conversation

Ocupe
Copy link
Contributor

@Ocupe Ocupe commented Nov 3, 2023

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:

export type UserChoices = {
  videoInputEnabled: boolean;
  audioInputEnabled: boolean;
  videoInputDeviceId: string;
  audioInputDeviceId: string;
  username: string;
};

Settings in local storage are updated by changes in the ControlBar and the PreJoin 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.

Copy link

changeset-bot bot commented Nov 3, 2023

🦋 Changeset detected

Latest commit: 9bb9fb1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@livekit/components-core Minor
@livekit/components-react Minor
@livekit/component-example-next Patch
@livekit/component-docs-storybook Patch
@livekit/components-docs-gen Patch

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

Copy link
Contributor

github-actions bot commented Nov 3, 2023

size-limit report 📦

Path Size
LiveKitRoom only 1.87 KB (0%)
LiveKitRoom with VideoConference 31.15 KB (+1.84% 🔺)
All exports 36.49 KB (+1.91% 🔺)

@lukasIO
Copy link
Contributor

lukasIO commented Nov 3, 2023

I like the idea, I'm wondering if the stuff that is in core right now, might actually make sense in livekit-client directly? That way not only react users would profit.

@Ocupe Ocupe requested a review from lukasIO November 3, 2023 16:11
@Ocupe Ocupe changed the title Add usePersistentDeviceSettings hook and device settings saving functionality Add usePersistentUserChoices hook to save device settings and username to local storage Nov 6, 2023
/**
* Whether to prevent saving user choices to local storage.
*/
preventSave: boolean = false,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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,
Copy link
Contributor

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;
Copy link
Contributor

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*

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about skipSave: boolean?

Copy link
Contributor

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!

@Ocupe Ocupe merged commit 01611f5 into main Nov 6, 2023
2 checks passed
@Ocupe Ocupe deleted the ocupe/add-persisted-device-info branch November 6, 2023 16:55
@github-actions github-actions bot mentioned this pull request Nov 6, 2023
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