-
Notifications
You must be signed in to change notification settings - Fork 44
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
adding shortcuts on the video player buttons controltext #1317
base: dev
Are you sure you want to change the base?
adding shortcuts on the video player buttons controltext #1317
Conversation
Your Testserver will be ready at https://1317.test.live.mm.rbg.tum.de in a few minutes. Logins
|
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.
You shouldn't create js files directly but rather create Typescript files and include them into the bundles so they get optimized. Please move your js code to the correct typescript files
If you still want to merge this PR please reopen the PR and fix the issues |
@@ -74,7 +74,7 @@ | |||
<p class="font-semibold">Signed in as</p> | |||
<span>@{{$user.Name}}</span> | |||
</header> | |||
<nav class="d-grid gap-3 font-light"> | |||
<nav class="d-grid gap-3 font-light" x-data="{showPopup: 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.
Are we sure we want this on every page and not just on the watch page? The shortcuts are only relevant there, right?
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 was only a rewrite of the existing, closed PR to better fit our codebase so it can be discussed again. Having said that, I agree. The question is where to incorporate this into the watch page? I'm not too happy about any other place and the menu where it currently is incorporated isn't included in the watch page.
Motivation and Context
fixes #1289
Description
Steps for Testing
Screenshots
It used to say only play, not the keyboard shortcuts are also displayed.



Keyboard shortcuts in menu:
Keyboard shortcuts popup: