search

quran / quran.com-frontend-next

Frontend build on next.js
https://quran.com
1.37k stars 399 forks source link

Add volume change functionality to audio player #2112

Closed mauwaz closed 6 months ago

mauwaz commented 6 months ago

Summary

User can now change volume of audio player without having to change system volume. Volume will persist on reload.

Test Plan

Known Issue:

Volume change not working on iOS mobile device due to Apple's policy.

Screenshots

Before After
image image
vercel[bot] commented 6 months ago

@mauwaz is attempting to deploy a commit to the Quran Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] commented 6 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
quran-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 18, 2024 8:20am
osamasayed commented 6 months ago

Alsalmau Alikum @mauwaz Jazak Allahu Khairan for the PR! mashAllah very neat UI and functionality. I am afraid there is already an opened PR that tries to add the same functionality

The above PR is still pending some improvements for it to be mergeable. Was wondering if you might be interested in working on the comments in the other PR better instead of creating a new one? Also, I found a couple of issues related to your PR:

  1. Volume controller is not working on Chrome on iOS device.
  2. Volume level does not persist when you refresh the page
mauwaz commented 6 months ago

Was wondering if you might be interested in working on the comments in the other PR better instead of creating a new one?

Thank you for heads up. While I appreciate the suggestion, I believe it would be more efficient to proceed with the new PR, as the existing one requires significant refactoring and would involve duplication of effort. 🤔

mauwaz commented 6 months ago

Pt 1:

As per Apple's documentation under section "Volume Control in Javascript":

On iOS devices, the audio level is always under the user’s physical control. The volume property is not settable in JavaScript. Reading the volume property always returns 1.

Pt 2:

Resolved

osamasayed commented 6 months ago

Was wondering if you might be interested in working on the comments in the other PR better instead of creating a new one?

Thank you for heads up. While I appreciate the suggestion, I believe it would be more efficient to proceed with the new PR, as the existing one requires significant refactoring and would involve duplication of effort. 🤔

I see. I just feel bad about the effort that has been duplicated already between this PR and the other one. My bad there is no clear features backlog for contributors to know which features are being worked on by whom. InshAllah we will improve this flow very soon. Anyways let's roll with this PR InshaAllah. Will be testing it a bit more and let you know If I have any comments.

mauwaz commented 6 months ago

Feedback addressed

osamasayed commented 6 months ago

Feedback addressed

Jazak Allahu Khairan!