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 a sound controller #21

Merged
merged 8 commits into from
Nov 22, 2022
Merged

Conversation

Daniel53245
Copy link
Contributor

@Daniel53245 Daniel53245 commented Nov 20, 2022

Closes #13

Describe the changes you've made

Add a volume controller on the website that sticks at the bottom.

Type of change

What sort of change have you made:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, local variables)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I went through the test serval time and the volume controller works well

Checklist:

  • My code follows the guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly wherever it was hard to understand.
  • I have made corresponding changes to the documentation.

@netlify
Copy link

netlify bot commented Nov 20, 2022

Deploy Preview for arito ready!

Name Link
🔨 Latest commit 83d16f6
🔍 Latest deploy log https://app.netlify.com/sites/arito/deploys/637b4646ae47120009c2fe20
😎 Deploy Preview https://deploy-preview-21--arito.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@prakhartiwari0
Copy link
Owner

prakhartiwari0 commented Nov 20, 2022

@Daniel53245

Kindly edit the Pull request and write the section where you need to Describe what changes you have done.
If you need help anytime, or have any query feel free to reach out!

@prakhartiwari0
Copy link
Owner

There are a lot of bugs currently, please also include recordings for proper analysis of the changes.

One bug I have got is that we cannot move forward after start the test, there are some errors that appear in the console when click on Start The test button -
image

@prakhartiwari0
Copy link
Owner

But this new feature is really great! I appreciate your hard work, it's functioning really well and of course, have some bugs as every code does have them, we need to solve these bugs together!

@Daniel53245
Copy link
Contributor Author

Daniel53245 commented Nov 20, 2022

Thanks, I am now testing it to see how those errors are triggered.
I just did a few modifications in the javascript, it should be free of those errors now.
Sorry for the bugs

 fix bug caused by the volume value out of bound
@prakhartiwari0
Copy link
Owner

Great Job @Daniel53245 !!
I am just testing it out from my side, on Chrome mobile, and on Brave & Firefox Desktop. Will tell you if I find any issues.

@prakhartiwari0
Copy link
Owner

And yeah I forgot to assign you this issue, sorry for that

@prakhartiwari0
Copy link
Owner

Hey @Daniel53245 what if we keep the controller at the bottom and not make it position fixed which results in it being overlayed over other elements. I think it would be good for UX don't you think so?

@prakhartiwari0
Copy link
Owner

And yeah I tested it, everything is working fine as per my tests. I am looking for some cross browser testing tools to test the app on various engines from one computer.

@Daniel53245
Copy link
Contributor Author

Hey @Daniel53245 what if we keep the controller at the bottom and not make it position fixed which results in it being overlayed over other elements? I think it would be good for UX don't you think so?

Yes, it is a good idea. We have to be careful while we are using elements that overlay at fixed positions. I might overload the user with redundant information.

From my perspective, it would be better to keep it fixed position. The volume slider is pretty tiny and will not likely disturb reading the content.

Moreover, if the application grows larger, the user would have to scroll back and forward to adjust the volume. Many web pages also use this kind of design for settings and volume. An example would be google.
msedge_1EAPrlbarr
As you could see in the above image they have the setting and the content you searched at the top.

Another reason that I put it at the bottom right is that the volume control on the windows system is at the bottom right of the screen. If someone feels that it is too loud, they would naturally move their mouse to the bottom right noticing the volume slider we provide.

Overall, I really hope that we could keep the volume slider at its current position. It is convenient for the user using the software. Moreover, we could do some further operations to design the graphic of the slider or try to hide it when unnecessary.

@prakhartiwari0
Copy link
Owner

I understand your points, on Desktop, there is no problem with that,
but when we open the app on mobile, this happens-

Screenshot from 2022-11-21 09-19-18 Screenshot from 2022-11-21 09-19-55

And because we are targeting both mobile and desktop users, this app needs to have good UI/UX for both devices. Also, we are using the mobile-first approach in Styling if you had noticed.
And therefore I was making the point of keeping it at the bottom. Because the app is not so long in terms of height, mobile users will not have any problem scrolling down. In fact, they need to scroll for volume only on the TEST FORM page and RESULTS page.
And the example of google might not be a good one here, because the only thing (at least the main task) we do at google is to search, and therefore it makes sense for them to make the search bar stick on the top so that the user doesn't need to scroll up the long Results Page (the Results page is long in terms of height) to use the search bar, which improves UX a lot.
If you go to DuckDuckGo, they haven't made the search bar stick to the top, which makes it quite
annoying to go to the top again and again just to search. (I have used DuckDuckGo's example just to explain)

But in our case, the volume button is just a feature and not the main element with which the user will be interacting most of the time, therefore keeping it at the bottom and not making it appear every time on top of other elements, makes sense.

I hope I was able to briefly explain my points, if you have any other points regarding the UI/UX I would be glad to know them!

@Daniel53245
Copy link
Contributor Author

j1EdjNaJw5
Hi, I just made the change on the repo. Thank you for pointing out that the design is a problem for screens with different ratios. Sorry for the inconvenience. Now the controller is outside the area the user interacts with the application.

@prakhartiwari0
Copy link
Owner

prakhartiwari0 commented Nov 21, 2022

Now it is better!
There are still some bugs we need to solve, but we will tackle them in the future.

This is one problem I found on the Results Page,

image

But don't worry, I have figured out the solution, and will implement it after merging this PR. I am currently learning about Versioning Software, Conventional Commits, and stuff to set up a versioning & releases system for this project. This would make things work better.

@prakhartiwari0
Copy link
Owner

So once I set up all of that, I will merge this PR which will make a new version of this app!

@prakhartiwari0
Copy link
Owner

prakhartiwari0 commented Nov 21, 2022

I once again want to appreciate your efforts and thanks for contributing to this project!
I hope you contribute more and make this open-source project a lot better with time 😄

@prakhartiwari0
Copy link
Owner

Hey @Daniel53245 please add a .gitignore file and write .DS_Store inside it. This file is coming with your PR and is not required. I guess it is a Mac config file or something.

Copy link
Owner

@prakhartiwari0 prakhartiwari0 left a comment

Choose a reason for hiding this comment

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

This feature is Amazing! Thanks to @Daniel53245 for making this contribution to the project.

@prakhartiwari0 prakhartiwari0 merged commit d62c031 into prakhartiwari0:main Nov 22, 2022
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.

[FEATURE] Add Sound/Music/Volume Buttons
2 participants