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 Music Player UI for Mobile Devices #2940

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Conversation

anshg1214
Copy link
Member

This PR adds a Music Player UI for Mobile Devices

@MonkeyDo
Copy link
Member

Looks awesome already! Much, much better UI on mobile, and I think the desktop version is also going to be jealous :P

Early feedback based on this:
image

  • I added a padding-bottom of the height of the brainzplayer bar (@brainzplayer-height + @progress-bar-height variables), otherwise the bottom buttons are all hidden behind it:
    image
  • You can't really read the track duration text; you might need a tool to decide which color is most readable, and we have tinycolor2 installed. You can generate complementary colors for example, and use this utility to automatically choose one with default to black/white if it's not readable: https://github.com/bgrins/TinyColor?tab=readme-ov-file#mostreadable I was just looking at that for LB-1102, LB-1610, LB-945: Revamp Top Entity stats pages #2937
  • I think the top header might be a bit too tall; it looks quite roomy, and (it's hard to see) the cover art is cropped at the bottom, suggesting there is not enough space available (because of the bottom padding I added)
  • Regarding the sizing of everything I would recommend the following:
    • Have a padding all around the player of 1em that will serve to align all the elements inside left and right
    • Remove the padding from .cover-art-scroll-wrapper and add the following to make sure the cover art stays at max dimensions: flex-shrink: 0; aspect-ratio: 1;
    • Remove horizontal padding for the other elements
    • Don't constrain the height of the .info and .header classes. Basically, we set a full size for the cover art and let the rest automatically take up the available space
  • I think the cover-art-wrapper class doesn't need to be flexbox (or maybe you do for swiping??).
  • The feedback icons stroke looks too thick next to the rest of the UI. stroke-width: 20px; (or maybe 1em) (down from 50px) should work:
    image
  • Finally, for .player-buttons you can remove the padding and the font-size, and set margin-top: auto; margin-bottom: auto; to automatically center it vertically in the available space. Each button inside can have the class .btn-icon added to have a larger button target for my fat fingers :P

The result is this, with flexbox highlighted for convenience

image

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

A few more little pieces of feedback as I've been testing on my phone.
The new player UI is looking sick !

frontend/css/brainzplayer.less Outdated Show resolved Hide resolved
frontend/css/brainzplayer.less Outdated Show resolved Hide resolved
const musicPlayerCoverArtRef = React.useRef<HTMLImageElement>(null);

React.useEffect(() => {
getAverageRGBOfImage(
Copy link
Member

@MonkeyDo MonkeyDo Aug 7, 2024

Choose a reason for hiding this comment

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

I have been looking at how we can improve this color detection stuff, not for right now but more generally.
I found a nice library that will extract multiple prominent colors of an image rather than just the average color:
https://jariz.github.io/vibrant.js/ (deprecated, but see the repo below taking over)
https://github.com/Vibrant-Colors/node-vibrant

That will allow us to use multiple colors from the image for the background and the text and be closer to the look and feel of the image, and preventing "issues" like this one, where the color isn't quite right and the white text is barely legible:
bp-mobile-color
Instead, it would be ideal if we could use the yellow from the image as background and the red color for text and maybe even the green for other elements of the UI.

Something for another day in any case, but could really improve the look

@MonkeyDo
Copy link
Member

MonkeyDo commented Aug 9, 2024

Looks like after I finish my queue, the player starts playing from the top despite the loop/repeat option being off

EDIT: Sorry I wrote this comment on the wrong PR ! :P

@anshg1214 anshg1214 marked this pull request as ready for review August 10, 2024 14:06
Base automatically changed from brainzplayer-spa to master August 13, 2024 15:15
@MonkeyDo
Copy link
Member

MonkeyDo commented Oct 9, 2024

I will let you resolve the conflict if you don't mind, and I'm ready for finalizing the PR (sorry I took so long coming back to this!).
Will do extensive testing locally and deploy on test as well to get aerozol's feedback (and maybe wider community after that?)

@MonkeyDo
Copy link
Member

MonkeyDo commented Oct 21, 2024

I deployed the PR to test.LB
As far as I can see, it's working really well.
There are three issues I see:

  • The top-right button doesn't seem to do anything
  • It's odd to have two faList icons that mean different things (bottom bar: opens the mobile player view, mobile player view: opens the queue), and act differently depending on if music is playing (opens the queue if no music is playing, opens the mobile view if music is playing). I think it's a bit confusing.
    My proposal to resolve the confusion: on mobile, in the bottom bar have the faCompactDisk icon that opens the mobile player view, instead of the queue icon. I would also make a cover art click (in the bottom bar) open the player view as well (currently opens a slightly bigger cover art image, good on desktop but not necessary now that we have the mobile player view).
    This means we can't open the queue directly with one button click on mobile, which to be honest matches what I've seen with other music players.
  • The queue is probably quite hard to close on mobile, now that the only way to close it on mobile is the small arrow button at the top-right. Might deserve a more generous button

And an extra follow-up issue for another day/another PR: with the mobile device horizontal, we will want to reorganize the layout to suit the aspect ratio better.

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