-
-
Notifications
You must be signed in to change notification settings - Fork 770
added seperate quality setting for full screen modes #3238
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
Conversation
|
thank you so much! @bashward |
|
related, untested: https://github.com/code-charity/youtube/pull/3141/files |
|
@ImprovedTube kept only one even listener in init js, let me know if you'd like any other changes. |
|
hi! @bashward they can stay, sorry, just meant, since we have many features we can make all feature code depend storage sorry ImprovedTube.storage.full_screen_quality !== 'auto' is even required. got to merge 95aeb5d There is another new feature requiring to react on fullscreen: https://github.com/code-charity/youtube/pull/3141/files , not sure it works. So maybe the listeners should be added in multiple cases after all
why? optionally, to allow uses to apply or remove a specific feature without a reload, we can react on the toggle immediately when it is clicked (not required/could be automated) youtube/js&css/web-accessible/core.js Lines 209 to 240 in c301a5d
|
|
Hi! @ImprovedTube, thanks for the explanation. So I'm thinking of adding a helper function in core.js in top level and in the existing storage change dispatcher, call that function kind of like: This would allow me to make the call in init js minimal and keeps the feature modular like: Let me know what you think about this. |
|
hi! @bashward, Options / later: reacting on storage change in core.js: only if we want to react in any way on toggle on or off immediately, which might be convenient, but most users are used to reload the page / are patient enough.) looking at the other full screen feature |
| "message": "Exclude Shorts when using \"Play all\"" | ||
| }, | ||
| "fullScreenQuality": { | ||
| "message": "Fullscreen quality" |
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.
This should close #3237
Changes:
full_screen_qualityfullscreenchange; restore on exit