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

Add volume change functionality to audio player #2112

Merged
merged 6 commits into from
Mar 28, 2024

Conversation

mauwaz
Copy link
Contributor

@mauwaz mauwaz commented Mar 14, 2024

Summary

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

  • Volume slider with auto-close after user pick a value (current delay = 1500ms)
  • Volume icon will update according to volume
    • For volume 0, mute icon will be displayed
    • For volume 1-49, low volume icon will be displayed
    • For volume 50-100, high volume icon will be displayed

Test Plan

  • Play audio of any Surah.
  • Click on volume icon.
  • Slide left or right to decrease or increase volume

Known Issue:

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

Screenshots

Before After
image image

Copy link

vercel bot commented Mar 14, 2024

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

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Mar 14, 2024

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

@mauwaz mauwaz force-pushed the feature/change-volume branch from ca87c25 to c0c467e Compare March 14, 2024 12:23
@osamasayed
Copy link
Member

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
Copy link
Contributor Author

mauwaz commented Mar 17, 2024

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
Copy link
Contributor Author

mauwaz commented Mar 17, 2024

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
Copy link
Member

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.

src/redux/Provider.tsx Outdated Show resolved Hide resolved
@mauwaz
Copy link
Contributor Author

mauwaz commented Mar 18, 2024

Feedback addressed

@osamasayed
Copy link
Member

Feedback addressed

Jazak Allahu Khairan!

@osamasayed osamasayed merged commit 8ebeff1 into quran:master Mar 28, 2024
4 checks passed
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.

2 participants