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

feat(audio controls): improve responsiveness of audio controls #554

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Jan 14, 2021

Changes

  • Hides: Shuffle, repeat and itemmenu on xs devices
  • Hides Volume control on lg and smaller devices
  • Decreases cover art size on smaller devices

Narrowest
image

Wider
image

Even wider
image

Wider still...

image

@codecov-io
Copy link

codecov-io commented Jan 14, 2021

Codecov Report

Merging #554 (b863387) into master (0211ccb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #554   +/-   ##
======================================
  Coverage    5.29%   5.29%           
======================================
  Files         104     104           
  Lines        2851    2851           
  Branches      428     428           
======================================
  Hits          151     151           
  Misses       2693    2693           
  Partials        7       7           
Impacted Files Coverage Δ
components/Layout/AudioControls.vue 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0211ccb...b863387. Read the comment docs.

@ferferga
Copy link
Member

Maybe not for this PR but what about moving shuffle, repeat, stop and such (not volume slider of course) to the item menu? I think that would be the best solution.

@camc314
Copy link
Contributor Author

camc314 commented Jan 14, 2021

I think skipping, and play/pausing are commands that are most likely to be used, this is pretty similar to spotify, the youtube mini player, and plex amp. If we were to move them to the item menu, it is still the same number of clicks as going to the full screen player. I'm open to other opinion, but I think that adding the item menu as well makes it a tad too cluttered.

If anyone has any other thoughts, I'd be interested to hear them

@ferferga
Copy link
Member

@camc314 Maybe I haven't explained myself

Wider still: As is right now
Even wider: As is right now
Wider: As is, but add menu item with the "Stop playback" option.
Narrower: As is, but add menu item with previous, repeat, shuffle and stop playback.

Maybe the options in narrower are the ones more up for discussion, but imo the stop playback should always be present.

I forgot in my PR and it would be nice if it's here instead of the one where I plan to expand more music features: click anywhere in the footer in the layout where there's no full screen button to open full screen music page.

@heyhippari
Copy link
Contributor

Narrower: As is, but add menu item with previous, repeat, shuffle and stop playback.

I agree with @camc314 for this, an item menu is kind of useless since one tap on the footer should bring you to the full screen and then those controls are available.

I would even hide the repeat and shuffle on the wider one, honestly, but that's up for discussion.

@ThibaultNocchi
Copy link
Member

I quite like the decisions @camc314 made on these screenshots. I think you could even add the stop playback button even on the narrower screens. And yeah clicking on the footer should put the player to full screen

@heyhippari heyhippari mentioned this pull request Jan 14, 2021
16 tasks
@sonarcloud
Copy link

sonarcloud bot commented Jan 17, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari heyhippari merged commit 1070872 into master Jan 17, 2021
@heyhippari heyhippari deleted the resp-aud branch January 17, 2021 07:07
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.

None yet

5 participants