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

Animations for player elements #1378

Closed
wants to merge 7 commits into from
Closed

Conversation

nvllz
Copy link
Contributor

@nvllz nvllz commented Aug 14, 2024

Changes:

  • Smooth transitions for song cover, title, and artist labels in the miniplayer
  • Limited line height for title/artist labels in player to prevent horizontal UI movement (found with Chinese fonts, fixes vertical animation bug)
  • Smooth transitions for title and artist labels in the player
  • Elapsed time label animation on manual seek
  • fixed rare crash caused by empty artist list
7e2564c5-608d-4a13-a092-32bacc82e5ca.mp4

@z-huang you might want to lower the slider refresh delay value as I was working on this pull request before your recent changes.

@z-huang
Copy link
Owner

z-huang commented Aug 14, 2024

  • The text animation in player screen looks great!
  • Why isn't the artist text in mini player animating?
  • I don't think the elapsed time text animation necessary. Sometimes I need to drag to a specific time, but with this animation, I have to wait a little time for it to stop animating.

@nvllz
Copy link
Contributor Author

nvllz commented Aug 14, 2024

Fixed both concerns, sorry for missing that.

@z-huang
Copy link
Owner

z-huang commented Aug 14, 2024

You shortened the elapsed time animation, but I still think it unnecessary. Animation doesn't need to be applied everywhere.

@nvllz
Copy link
Contributor Author

nvllz commented Aug 14, 2024

Okay, if that's your only concern, feel free to drop that elapsed time label animation. But now I don't think it's really something that can make people "wait" because it's kind of like it was before when you drag it. Your choice, zhuang.

@z-huang
Copy link
Owner

z-huang commented Aug 15, 2024

Using a fixed height may not be good for all case. If the text is bigger, it's cut.

Screenshot_2024-08-15-09-42-30-085_com zionhuang music debug

@nvllz
Copy link
Contributor Author

nvllz commented Aug 15, 2024

If you don't apply the line height limit, the whole UI will move when larger text is displayed, and you'll get a vertical animation for those elements as before. If possible, setting it to the same value as current font size + 3.sp would work best from what I tested. If not, consider changing a font or idk.. Set a fixed font size and prevent it from scalling, as you probably don't care about these when using a bigger font size:

fe182df2-b819-49e2-8df2-dbd78bc8d2b4

@z-huang
Copy link
Owner

z-huang commented Aug 20, 2024

As for the menu, I'm also thinking about changing the style to list. I found an animation that I prefer more: forward and backward https://m3.material.io/styles/motion/transitions/transition-patterns

@z-huang
Copy link
Owner

z-huang commented Aug 27, 2024

These animations can be done by #1017, so I'll close this pr. Thanks for your contribution!

@z-huang z-huang closed this Aug 27, 2024
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