mwalbeck / nextcloud-breeze-dark

A Breeze Dark theme for Nextcloud
GNU Affero General Public License v3.0
312 stars 56 forks source link

The theme breaks the play/pause indicator logic in the Music app #28

Closed paulijar closed 5 years ago

paulijar commented 6 years ago

I'm the author of the Music app and noticed the following problem while investigating (an unrelated) bug reported for the app:

When the nextcloud-breeze-dark theme is enabled, the Music app shows the play indicator in front of each track and each view/playlist name: breeze-dark

The indicator is supposed to show only on the current track and on the hovered track. In the navigation pane, the indicator should be shown on the currently playing view and when hovered directly on the indicator.

The problem is in how the theme overrides the rules of the class .play-pause. To work correctly with the latest version of the Music app (v0.8.0 released a few hours ago), the rules should be as following:

.current.playing .play-pause,
div:hover > .play-pause,
#app-view .current:not(.playing) div:hover .play-pause,
#app-navigation .current:not(.playing) :hover .play-pause {
    background-image: url('../img/actions/play-big.svg');
}
.current:not(.playing) .play-pause,
#app-view .current.playing div:hover .play-pause,
#app-navigation .current.playing :hover .play-pause {
    background-image: url('../img/actions/pause-big.svg');
}

The most important thing to note here is that there is no rule for the class .play-pause alone. This is because the element is essentially a 3-state indicator where the icon may be play, pause, or none. The none state is the default and to show other icons there are additional conditions.

Note that the Music v0.8.0 made some changes on the track item structure so the style definitions given above don't work perfectly on older versions. It would probably be impossible to define the rules so that they would work 100% correctly on all versions. But just to be clear, the problem described here was present already on older versions of the Music app.

Test configuration: Theme version 13.0.4 Music app version 0.8.0 and 0.7.0 Nextcloud version 14.0.0-alpha (shouldn't be relevant)

paulijar commented 6 years ago

As an unrelated issue, you might want to override also the background-color on the Music app elements #app-sidebar .close and #app-sidebar #follow-playback. These are part of the track details pane just added in the version 0.8.0 and can be seen in the top-right and bottom-right corners of the following screenshot: breeze-dark

These elements are supposed to have the same background color as is the window background. They have non-transparent backgrounds because it's possible that these elements with fixed position will overlap other elements.

mwalbeck commented 6 years ago

Thanks for the details, should be fixed now.

I looked a bit closer at the generic .play-pause rules in the theme and they were originally written for the Audio Player app. So I'll need to take a look and make sure there are no further conflicts between the two.

Much appreciated.

paulijar commented 6 years ago

Okay, great! I tested the top of the develop branch and seems to work fine now.

I didn't realize that Audio Player had element with the same class name, but I guess that odds for such collision were quite high.

paulijar commented 6 years ago

Here's one more heads up on the Music app. Yesterday, I released a new version 0.9.0 which introduced two new css definitions:

.highlight {
    background-color: rgba(0,0,0,0.025);
}

and

#app-navigation li.drag-hover {
    background-color: rgba(0,0,0,0.04);;
}

The first is supposed to be a very faint highlight around current artist or album after playing has been started by clicking the artist/album title. The second is used to highlight the playlist name on the navigation pane while a track is being dragged to that playlist.

You might want to override these rules on the dark-breeze theme. At the moment, the first of these is completely invisible and also the second one is difficult to see with the theme. I would say that these are not critical problems, though.

image

image

mwalbeck commented 6 years ago

Thanks for the heads up, I'll take a look at it.

mwalbeck commented 5 years ago

Took some time to get to it, but it is now included in the latest release.

Thanks.