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

[DRAFT] Demo: store the settings in the url hash #1349

Open
wants to merge 89 commits into
base: dev
Choose a base branch
from

Conversation

Florent-Bouisset
Copy link
Collaborator

@Florent-Bouisset Florent-Bouisset commented Jan 5, 2024

Storing the player settings in the url hash let us refresh the demo page while keeping all the settings previously set.
It's more efficient and comfortable when debugging to not have to reset them manually after each page refresh.

To Do:

  • Add a switch/checkbox to disable/enable the storing in url OR let the RxPlayer logo be a tag with href with an empty hash allowing to reset.
  • Reloading a preset content will display it as a custom content, we can eventually improve that

@peaBerberian
Copy link
Collaborator

Add a switch/checkbox to disable/enable the storing in url OR let the RxPlayer logo be a tag with href with an empty hash allowing to reset.

I would advocate for both:

  • the switch would stop the URL from updating but keep it where it was. This would be useful when debugging settings you know are problematic, then playing with them, while being able to reset to the problematic scenario by just refreshing the page
  • the link would just reset to the default

const loadContent = React.useCallback((content: IPlayableContent) => {
getLoadVideoOptions(content).then(
(loadVideoOptions) => {
loadVideo(loadVideoOptions);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't look too much into it yet but have you verified that this code is not sensible to race conditions?

For example, if loadContent is called multiple consecutive times, are we sure that the last call will lead to playback in the end?

Will look more into it, but that's a question I was asking myself reading this

Copy link
Collaborator

Choose a reason for hiding this comment

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

(the issue may already have been there before)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to provoke a race condition but didn't succeed in doing so.
I still have added the code to prevent any race condition, let me know what you think of that because I'm not 100% sure that this is the correct way of doing so.

@peaBerberian peaBerberian changed the base branch from next-v4 to dev January 25, 2024 11:38
@peaBerberian peaBerberian added Demo Relative to the RxPlayer's demo page Priority: 2 (Medium) This issue or PR has a medium priority. labels Feb 5, 2024
@peaBerberian peaBerberian added this to the 4.1.0 milestone Feb 5, 2024
peaBerberian and others added 24 commits March 4, 2024 11:55
…ite-spinner

fix(directfile): media with autoplay:false is infinitly in loading state
When doing some hotfixes for our v4.0.0 version, we directly integrated
a browser detection check in the main code of the RxPlayer.

The normal way for the RxPlayer of handling device compatibility, is to
clearly list and document all differences in the `src/compat` directory,
not directly in the main code.

This commit fixes that by giving a name to that work-around, documentating it
and moving it in the `src/compat` directory.
…from-initial-seek-and-play

Remove `isSafariMobile` mention from `main_thread` code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Demo Relative to the RxPlayer's demo page Priority: 2 (Medium) This issue or PR has a medium priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants