Skip to content
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

Better progressive playback controls of bottom pages #164

Merged
merged 13 commits into from
Mar 5, 2019
Merged

Better progressive playback controls of bottom pages #164

merged 13 commits into from
Mar 5, 2019

Conversation

xlinbsd
Copy link
Contributor

@xlinbsd xlinbsd commented Mar 1, 2019

The volume slider is important to conserve at most width for audio playing.
The new value of hiding seems best balanced.

Only tested on last Firefox on Linux. Other tests for a better compromise are welcome.

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 1, 2019

Tested with latests Linux Chromium and Electron OK too.

@xlinbsd xlinbsd changed the title Allow volume change on shrinker width screenview Better progressive music control panel display Mar 1, 2019
@xlinbsd xlinbsd changed the title Better progressive music control panel display Better progressive music control nowplayingbar Mar 1, 2019
@sparky8251
Copy link
Contributor

Do be aware, there are 2 other places in code that the volume slider exists. Its also in videoosd.js and remotecontrol.js.

Might not have to touch the videoosd.js one, but the remotecontrol.js is actually used in the second music playback screen. The one where you have your queued playlist along the bottom.

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 1, 2019

@sparky8251 OK Seen. Thanks

Keeping more preferences toward essential controls in last.
Mostly granular show/hide now

I can generalize this modifications into other playback control zones in another pull if you are ok. This one seems a priority from my point of view, my personnal usage.
@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

Hello, is the new ordering please anyone ?

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

I included my modifications of second pull since it is in the same objective to improve control bars

@sparky8251
Copy link
Contributor

Have any before and after pictures?

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

@sparky8251 I'll do it

@xlinbsd xlinbsd changed the title Better progressive music control nowplayingbar Better progressive music control bottom page bars Mar 2, 2019
@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

After...

after
The spacing seems wide on the left, but titles can take a lot of space, more than my example, we need to let it clear of widget

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

Before.

before

@xlinbsd xlinbsd changed the title Better progressive music control bottom page bars Better progressive playback of bottom pages Mar 2, 2019
@xlinbsd xlinbsd changed the title Better progressive playback of bottom pages Better progressive playback controls of bottom pages Mar 2, 2019
@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

Tested on latest iOS Safari, Lineage 16, macos 13's Safari, Linux Firefox && Linux Chromium

@sparky8251
Copy link
Contributor

sparky8251 commented Mar 2, 2019

It's looking good. Let us know when you feel its "complete". With that, we can properly review it.

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 2, 2019

I think it is complete for now. Need other users testing (in 10.2.3 ?) the new control style to be sure it is better for majority of us, no ?
I'll stay close if you think I can help a bit, I'm on the Riot room. I do know more than just some CSS coding.

Copy link
Contributor

@vitorsemeano vitorsemeano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving since it was my assumption that was incorrect

@media all and (max-width: 55em) {

.nowPlayingBarCenter .nextTrackButton, .nowPlayingBarCenter .previousTrackButton, .nowPlayingBarCenter .playPauseButton {
display: none !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you try without the !important? can't see to find more classes that use this sub classes with a display none important.

Also, nowPlayingBarCenter is parent of some of those classes. Try removing all sub classes of this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forget that last part, you are applying the rules to the sub classes, not the parent one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never myself use !important. That is a residue of the old code. I'll try without, it is a good idea.

@@ -56,7 +56,7 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader',

html += '<button is="paper-icon-button-light" class="muteButton mediaButton"><i class="md-icon">&#xE050;</i></button>';

html += '<div class="sliderContainer nowPlayingBarVolumeSliderContainer hide" style="width:100px;vertical-align:middle;display:inline-flex;">';
html += '<div class="sliderContainer nowPlayingBarVolumeSliderContainer hide" style="width:150px;vertical-align:middle;display:inline-flex;">';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like !important. could that be because of this inline style? display inline-flex? perhaps moving this rules to nowplayingbar.css will ease the !important? i could be asking too much already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check if !important is really important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some tests, !important is needed in this state of code. I will check why

@xlinbsd
Copy link
Contributor Author

xlinbsd commented Mar 3, 2019

Sorry for the last pushes, I was editing directly in github since I decided to do it right with git commands from my pc. I missed some editings while trying to do things right.

Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Bond-009 Bond-009 merged commit 89be94f into jellyfin:release-10.2.z Mar 5, 2019
@xlinbsd xlinbsd mentioned this pull request Mar 5, 2019
@xlinbsd xlinbsd deleted the patch-1 branch March 5, 2019 13:04
Bond-009 added a commit that referenced this pull request Mar 5, 2019
 Better progressive playback controls of bottom pages (#164)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants