feat: first pass of playback refactoring #1866
Conversation
ferferga
force-pushed
the
vite
branch
17 times, most recently
from
cc3f507
to
3b8f2c1
Compare
ferferga
force-pushed
the
playback-minor-improvements
branch
4 times, most recently
from
f24a4e5
to
5296316
Compare
ferferga
force-pushed
the
playback-minor-improvements
branch
from
5296316
to
ec1af91
Compare
ferferga
force-pushed
the
playback-minor-improvements
branch
from
ec1af91
to
506a581
Compare
ferferga
force-pushed
the
playback-minor-improvements
branch
from
506a581
to
5c91474
Compare
ThibaultNocchi
force-pushed
the
playback-minor-improvements
branch
from
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
ThibaultNocchi
force-pushed
the
playback-minor-improvements
branch
2 times, most recently
from
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
ThibaultNocchi
force-pushed
the
playback-minor-improvements
branch
from
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
ThibaultNocchi
force-pushed
the
playback-minor-improvements
branch
from
537a677
to
9e4b524
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Cloudflare Pages deployment
|
Doesn't work in development because vuejs/core#7567