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

Layout Improvements #755

Open
wants to merge 38 commits into
base: next
Choose a base branch
from
Open

Layout Improvements #755

wants to merge 38 commits into from

Conversation

Dumeng
Copy link
Contributor

@Dumeng Dumeng commented Dec 28, 2021

A batch of layout and UI improvements:


  • Improve responsive design
  • Layout cleanup

@Dumeng Dumeng marked this pull request as ready for review December 29, 2021 12:39
@listen1
Copy link
Owner

listen1 commented Dec 29, 2021

Thanks for so much work delicated to project. By the way, one pr for one feature is recommended. Easier to merge and modify.

Good to go.

Suggstion: Keep same align style for now playing playlist and common playlist.

  • Use grid to replace flex layout for playlist list view

Good to go. More elegant way to responsive design👍

  • Add default placeholder playlist cover

Question: I notice inline background image uses inline style base64. Will that be performance issue for many images?

This optimaze is necessary. And it's also complicated for design especially in really narrow width screen. I think the layout in narrow screen can be better designed. Or we can limit a min width.

Bug: adjust width of screen, play, next, prev button is not clickable.

I'm not sure increase playerbar height is for this requirement. It seems too high in my test environment (compare to height of sidebar entries).

  • Add platform label back for the search results

Bug: Seems not working.

  • Add Toggle Button component for settings

Necessary improvement. I think underline style seems a little strange because seldom design use that besides tab bar.

Style suggestion: change underline style to radio button or use iOS style (different background for highlight)

Style suggestion: change checkbox to button group is harder for users to understand. I recommend to use checkbox and we can change icons to more common checkbox icon as they used in netease/qq application.

Blur area seems too large. Maybe we can refer to netease/qq effect to adjust parameter

  • Refactor code structure of the settings page

Good to go

  • Make the settings page responsive

Good to go

  • Dynamically import static assets

Good to go

BTW: I'm not good at design, so these design suggestions are not professional. I think what I can do is find out some style many music player applications are using and try to follow them. It's really hard for me to judge a pretty new style is good for users to understand and use.

@Dumeng
Copy link
Contributor Author

Dumeng commented Dec 30, 2021

Suggstion: Keep same align style for now playing playlist and common playlist.

Fixed

Bug: adjust width of screen, play, next, prev button is not clickable.

Fixed

Question: I notice inline background image uses inline style base64. Will that be performance issue for many images?
Refer to (Vite docs for static asset)[https://vitejs.dev/config/#build-assetsinlinelimit]

This may makes final html structure a little messy, but I didn't see performance issue for now.

Style suggestion: change underline style to radio button or use iOS style (different background for highlight)

Updated

I'm not sure increase playerbar height is for this requirement. It seems too high in my test environment (compare to height of sidebar entries).

Looks fine for me. Do you have any screenshot can share?

Style suggestion: change checkbox to button group is harder for users to understand. I recommend to use checkbox and we can change icons to more common checkbox icon as they used in netease/qq application.

I attempt makes the style being more unified. I suggest we can use a standalone toggle button style from android design.
image

Blur area seems too large. Maybe we can refer to netease/qq effect to adjust parameter

The -webkit-mask-image feature is still experimental. Actually, I blur the lyric line by line. What means 'blur area too large'?

@mikelxk
Copy link
Collaborator

mikelxk commented Dec 30, 2021

image

It seem that the setting page will break if I set zoom level to 150%. The bug only occur when I set auto switch source to true.

The -webkit-mask-image feature is still experimental. Actually, I blur the lyric line by line. What means 'blur area too large'?

I believe it means the blur radius is set too high.
Also, I think the lyric should not be blurred when they are selected or scrolled.

@listen1
Copy link
Owner

listen1 commented Dec 30, 2021

About blur effect explain

Image 1. Blur effect from netease(mac) music player
截屏2021-12-30 下午10 53 42

Image 2. Blur effect from PR
截屏2021-12-30 下午10 56 12

There're some differences.

  1. Blur area.
    netease only blur top/bottom small area
  2. Blur effect
    netease only have very strong blur effect on top/bottom

@mikelxk
Copy link
Collaborator

mikelxk commented Dec 30, 2021

Setting Layout still look broken on 150% zoom level for me.
image
Also, when I enable comment,
image
The page is also broken.
I've also attached apple music dealing of lyric blur, as I see, the best implementation so far.
image

Fix tsc error

Fix Nowplaying page layout

Add the new blur effect
@Dumeng
Copy link
Contributor Author

Dumeng commented Jan 4, 2022

Setting Layout still look broken on 150% zoom level for me.

This is expected. You screenshot is looks like an ultra-wide window view, so the flex flow is extend horizontal. Refer to this link

And I also implement the blur with the experimental CSS masks feature. I didn't find a better way to implement this gradient effect. Refer to this link.

src/components/TrackRow.vue Outdated Show resolved Hide resolved
src/views/Settings.vue Outdated Show resolved Hide resolved
Didn't figure out any obvious effect. Can you explain more?

Co-authored-by: MikeLxk <[email protected]>
@mikelxk mikelxk mentioned this pull request Jan 18, 2022
22 tasks
@mikelxk
Copy link
Collaborator

mikelxk commented Jan 22, 2022

Could you extend the view window for lyric
image
That's the current window size, it seems that I can only see 5 lines of clear text in my current setting, medium font weight and 20px.
image
Also, the highlighted lyric is a little bit under the center of cover art, it would be very great if you could align to the center or above the center since the there's more lyric under.

@listen1
Copy link
Owner

listen1 commented Jan 23, 2022

Sorry for merge delay. This pr is too big to be merged for one time, especially many modification made and features included. It's beyond my ability to ensure all the things go well after merge.

Please submit pr for one feature, or UI adjust, or code refactor. It's easier to test and make further commit.

Thank you for your understanding.

@Dumeng
Copy link
Contributor Author

Dumeng commented Jan 23, 2022

I will try to do cherry-picks. But many reviews and modifications are made, I'm not sure this will work well.

@mikelxk mikelxk mentioned this pull request Feb 2, 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.

3 participants