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

Save caption state in localStorage #313

Closed
wants to merge 1 commit into from

Conversation

alethea
Copy link

@alethea alethea commented Jul 16, 2016

As mentioned in #311, captions aren't stored in between page loads. This is an attempt to refactor localStorage usage to accommodate saving captions on/off state when the user hits the captions button.

@sampotts
Copy link
Owner

Hey, thanks for this. I was just looking through though and thinking it might be best to keep all the settings under a plyr key in the storage and JSON.stringify() and JSON.parse() an object. We'll be adding more to the storage as time goes on and it'd be good to keep them tidy.

@alethea
Copy link
Author

alethea commented Jul 21, 2016

That sounds good to me. I was thinking of doing that in this PR anyways but I didn't want to make a big change if that wasn't the direction you were going. I'll refactor this tonight.

Also update localStorage to use a JSON object store for future
extensibility.
@alethea
Copy link
Author

alethea commented Jul 22, 2016

I rebased my branch to pull in the latest changes and reworked my implementation to use a JSON store inside localStorage. I chose to make the key captionsEnabled to allow for future language selection or something under a different captions key.

@sampotts
Copy link
Owner

This looks spot on - nice work 👍

I'm just in the middle of upgrading the Vimeo API but will merge this with that and release today or tomorrow.

Thanks!

@alethea
Copy link
Author

alethea commented Jul 22, 2016

Awesome! Thanks so much!

sampotts pushed a commit that referenced this pull request Aug 20, 2016
@sampotts
Copy link
Owner

Hey, thanks for this - I've manually merged this into some other refactoring work I've been doing. 👍

@sampotts sampotts closed this Aug 20, 2016
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.

None yet

2 participants