-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
feat: Full screen music UI, playback and home header improvements #520
Conversation
3dc6ddc
to
039a643
Compare
Codecov Report
@@ Coverage Diff @@
## master #520 +/- ##
=========================================
- Coverage 0.59% 0.56% -0.04%
=========================================
Files 88 89 +1
Lines 2344 2492 +148
Branches 355 372 +17
=========================================
Hits 14 14
- Misses 2329 2477 +148
Partials 1 1
Continue to review full report at Codecov.
|
fe15bb6
to
f9600d7
Compare
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.
@@ -2,6 +2,7 @@ | |||
<v-app ref="app"> | |||
<backdrop /> | |||
<v-navigation-drawer | |||
v-if="$store.state.page.showNavDrawer" |
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.
Why hide the nav drawer?
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.
@camc314 it's a full screen player.
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.
@ferferga Could it maybe be tied to fullscreen-ness of the player? Also the drawer already has a drawer
model and it could be seen as redundant?
Maybe even have an event being emitted from the audio controls to play with the drawer model directly?
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.
I agree with Thibault, v-model already does this and it's be better to use that instead
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.
For future reference, and as it was discussed in Matrix, we're going to leave this as an v-if
so toggling the nav drawer doesn't mess with the user's behaviour. Going the v-model route would require to store another variable that contains whether the user has the navdrawer hidden or not before toggling ot, so we can act accordingly after unhiding it.
If we want to switch everything to v-model at some point, we need to take care of that.
<transition name="fade-fast" mode="in-out"> | ||
<v-tooltip v-if="!isFullScreenPlayer" top> | ||
<template #activator="{ on, attrs }"> | ||
<nuxt-link tag="span" :to="'/playback'"> | ||
<v-btn icon class="ml-2" v-bind="attrs" v-on="on"> | ||
<v-icon>mdi-fullscreen</v-icon> | ||
</v-btn> | ||
</nuxt-link> | ||
</template> | ||
<span>{{ $t('fullScreen') }}</span> | ||
</v-tooltip> | ||
</transition> |
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.
What is wrong with expanding the modal to full screen?
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.
@camc314 Swiper's weirdness, DOM performance (slides are expensive, and with items in the background, much worse) and a bug in Vuetify where the scrollbar doesn't disappear and allowed you to scroll the things that were below the v-dialog.
components/Layout/AudioControls.vue
Outdated
isFullScreenPlayer(): boolean { | ||
return this.$route.name === 'playback'; | ||
}, |
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.
Why not use isMinimized?
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.
@camc314 If going from /playback to / directly in the address bar, the store doesn't get the mutation, so the fullscreen UI is still there.
I also reproduced this when switching between users/servers. We should maybe make an store call on page reload so everything gets cleared (but login info and DisplayPreferences), but that's for another PR.
This is meant to be a full screen, kiosk-like experience :p There's a separate queue screen planned with a different view. Main goal here is to just show the album art and give basic info and controls, to show on like a TV screen or a wall tablet or something. |
@camc314 Are you able to reproduce the off-center issue with the latest force-pushed changes? Original commits already had the hack enabled, but not on mount. Check nolimits4web/swiper#886 |
Yes, no change |
5daf4ba
to
4ad4de4
Compare
Forgive me, I'm don't think I quite understand. Is this layout going to be used for TVs and full screen desktops then, and a different one used for mobile and non-full screen desktops? |
802445e
to
cd0fadb
Compare
@camc314 This is similar to what YouTube Music, Tidal and Spotify has. It's just a page where you can leave the player sitting there while relaxing :P It's like opening VLC, it also displays the cover at full screen. Some example photos from other services: |
cd0fadb
to
4951b0c
Compare
b33c75d
to
220f169
Compare
Ok, that makes more sense |
220f169
to
dbdab28
Compare
3a82b94
to
9856d87
Compare
dfd0778
to
a23ed31
Compare
…ists and mute/unmute ability
a23ed31
to
508ae4c
Compare
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
Changes
BlurhashImage
toimageHelper
.2021-01-08.13-52-45.mp4
2021-01-08.13-56-48.mp4
2021-01-08.13-46-09.1.mp4
To do
Updates
Changed the miniplayer design between commits:
Full screen controls:
Active controls (with elevation):
Reduced window: