feat: first pass of playback refactoring #1866
Conversation
cc3f507 to
3b8f2c1
Compare
f24a4e5 to
5296316
Compare
5296316 to
ec1af91
Compare
ec1af91 to
506a581
Compare
506a581 to
5c91474
Compare
5c91474 to
0c08d8d
Compare
| * Once the user releases the slider, change the time of the playbackManager with whatever | ||
| * input value was provided by the user | ||
| */ | ||
| function releaseMouse(): void { |
There was a problem hiding this comment.
What about using a name describing what does the function instead of when it is supposed to be called? Like setNewTime or whatever. Now this isn't important anymore, but previously we had many events, some referring to the same function. That's why I think event functions should be named for what logic they do, not on what event they are called
There was a problem hiding this comment.
It's indicated in the comments though. Which name do you suggest?
There was a problem hiding this comment.
I don't know, something more related to what its role is, not its event. So something like setNewTimeFromSlider or whatever. The comment is great, but when seeing the function called in the template, you can't directly know what it does.
|
|
||
| const sliderValue = computed({ | ||
| get() { | ||
| return playbackManager.currentVolume; |
There was a problem hiding this comment.
Why drop the isMuted logic?
There was a problem hiding this comment.
I see this comment in time slider, not in volume slider. What do you mean?
There was a problem hiding this comment.
Before your commit, the slider had this prop: :model-value="playbackManager.isMuted ? 0 : playbackManager.currentVolume". Is it intended you dropped the fact that the slider is (virtually) at 0 when muted?
| }); | ||
|
|
||
| /** | ||
| * If the player is a video element and we're |
There was a problem hiding this comment.
/**
* If the player is a video element and we're
*/
const teleportTarget = computed<'body' | '.video-container'>(() => {
return mediaElement.value === 'audio' ? 'body' : '.video-container';
});
if that can help you :)
There was a problem hiding this comment.
@ThibaultNocchi IIRC I wanted to continue with something like this:
If the player is a video element and we're in the PiP player or fullscreen video playback, we need to ensure the DOM elements are mounted before the teleport target is ready
eb9a801 to
e0c39d8
Compare
| }); | ||
|
|
||
| /** | ||
| * If the player is a video element and we're |
There was a problem hiding this comment.
@ThibaultNocchi IIRC I wanted to continue with something like this:
If the player is a video element and we're in the PiP player or fullscreen video playback, we need to ensure the DOM elements are mounted before the teleport target is ready
e0c39d8 to
537a677
Compare
* Don't use async storage * Use arrow functions for TypeScript compatibility with this * Add store keys as constants * Fix defaultState being mutated using Object.assign as well, create a clone with lodash's cloneDeep
* This commit lays the groundwork and adds some hints to some files, with ideas of how I see those features reimplemented for playback. * Only music works at this point, but without AudioContext or any fancyness. * Change folder layout from Players to Playback
Backdrop was breaking v-app's behaviour. The order of the elements is important for things like v-hover to work properly
537a677 to
9e4b524
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Cloudflare Pages deployment
|
Doesn't work in development because vuejs/core#7567