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

Fix volume slider behavior #117

Merged
merged 14 commits into from
Feb 13, 2019

Conversation

sparky8251
Copy link
Contributor

Volume sliders now adjust volume when dragged (used to to only work when you stopped clicking).

Fixes #113

@sparky8251
Copy link
Contributor Author

Hmm... I guess this isn't properly complete yet. Further testing has shown that just plain clicking is no longer enough to adjust volume.

I'm not well versed in JS, can I add the original click event to nowPlayingVolumeSlider.addEventListener("mousemove", function () { so its effectively an OR?

And since Android also relies on jf-web, It will need testing before we want to merge this in.

@cvium
Copy link
Member

cvium commented Feb 2, 2019

I'm not well versed in JS, can I add the original click event to nowPlayingVolumeSlider.addEventListener("mousemove", function () { so its effectively an OR?

You can add as many event listeners as you want with separate calls eg.

nowPlayingVolumeSlider.addEventListener("mousemove", function () {
    dostuff();
});
nowPlayingVolumeSlider.addEventListener("click", function () {
    dostuff();
});

@sparky8251
Copy link
Contributor Author

This now works as one would expect on web. I am positive it will work as expected on Android, but I would sleep better if @dkanada or @thornbill would test this PR first.

Now ready for full review if anyone has been holding off!

src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/scripts/videoosd.js Outdated Show resolved Hide resolved
src/components/remotecontrol.js Outdated Show resolved Hide resolved
src/components/remotecontrol.js Outdated Show resolved Hide resolved
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.

Code seems functionally fine.
Waiting to see response on my feedback on de-uglification before stamping approval.

Updates volume correctly as you slide back and forth, but does not change icon to mute when at 0 or update bar length.

Has no impact on music playback.
Same problems as video when on nowplaying.html, all other pages are still behaving as before (when the playback bar is along the bottom).
Now updates when dragging
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.

Please answer my de-uglification comments as well.

.gitignore Show resolved Hide resolved
@sparky8251
Copy link
Contributor Author

Pending responses to my questions. Changes marked resolved are now pushed or were tested and they broke things.

Also, confirmed that there does not appear to be a bug with slider behavior like I originally thought. Once the suggested changes are in place this is mergable imo.

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 pending answer to a couple of unresolved conversations.
They could be safely ignored for now if wanted, though, as they're minor in a sense.

@sparky8251
Copy link
Contributor Author

Ok. All suggestions by @cvium and @JustAMan have been tested and implemented.

I haven't tested with this Android, but it works perfectly with FF and Chrome.

If no one has any other suggestions, I'd say this is ready for merge.

@joshuaboniface joshuaboniface merged commit fccf422 into jellyfin:master Feb 13, 2019
@sparky8251 sparky8251 deleted the volume-slider-fix branch February 23, 2019 01:04
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.

5 participants