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

[Redesign] Song menu tweaks #663

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

Komodo5197
Copy link

Used the same masking technique as the song queue to prevent text overlap in song menu. Slightly re-arranged options. Increased default menu height when opened - does the existing behavior of starting halfway up make sense on larger phones? To me it's fairly irritating because it means needing two gestures on both open and close to access the majority of the menu options.

@Chaphasilor
Copy link
Collaborator

This is how it looks on a taller screen:

Screenshot_20240331-115240.png

Screenshot_20240331-115258.png

We can probably increase the default extent by quite a bit, but it shouldn't be as much as the queue panel imo.

I'll have to take a look at the reordering once I get home; part of the current order was based on feedback.

In general, we could probably use a similar approach in the menu and queue panel like we did on the player screen for smaller screens, with the layout controller. I was thinking of shrinking the "song preview/info" section in the menu and un-stickying the current track in the queue.

@Chaphasilor
Copy link
Collaborator

Okay, just looked at it.

  • I don't see a reason to limit the maxChildSize, especially on smaller screens we can use all of the available space
  • It would be nice if we could dynamically set the initial size in a way that shows all menu entries if possible, but not any additional white space. But I guess that's only possible with some unelegant calculations? The screen height could be used to get the needed parent percentage.
  • Would it be possible to use a SliverAppBar for the song info, that contains a FlexibleSpaceBar, so that the song info and PlaybackActions can shrink when scrolling is needed, like on the album and artists screens?
  • The reordering looks good
  • Just saw the todo about the download button, I've also wondered about this. I think it should be consistent with albums, etc., where if the item is downloaded transitively, we just show the lock icon. In this case we could add a text label like "keep on device" instead of "Download". What do you think?

My "required" changes are just removing the maxSize and somehow reducing the initialSize for large screens. Everything else is optional for merging, but would make me happy ^^

…sign-song-menu

# Conflicts:
#	lib/components/AlbumScreen/song_menu.dart
#	lib/l10n/app_en.arb
@Komodo5197
Copy link
Author

Okay, I've put in a dynamic calculation to try to open the menu to exactly the right height, and it seems to work well enough. I also removed the maxSize and tried to clean up the download button to match the main DownloadButton behavior. Regarding the song info shrinking, there was already some behavior along those lines before. I suppressed it because the way it scrunched the album cover was ugly and I couldn't figure out how to fix it. Plus, even at full size it's only a 150px header, which is way better than the album screen. speaking of that, what do you mean about the album/artist screens shrinking?

@Chaphasilor
Copy link
Collaborator

Nice, everything works well!

What I mean was the following behavior we had in the stable version:

2024-04-01-00-05-55.mp4

That is not quite the behavior I would like (it should shrink and not just fade out), but it's using the flexible section of an AppBar.
A closer approximation would be the redesigned album screen: #220 (comment) (notice how the cover shrinks to take up less space).
I think that should be doable with something like https://api.flutter.dev/flutter/material/FlexibleSpaceBar-class.html, but maybe we need something more custom. Figuring that out is also needed for the album screen, so I'll take a look eventually, unless you figure it out first :D

Should I merge this now?

@Komodo5197
Copy link
Author

It had some sort of shrinking behaviour before, but the album shrunk out of the corner it's nested in, and the text didn't shrink and ended up with 0 padding, and it just didn't look great, so I just disabled it by setting the minimum size to the max. I don't really have a vision of what good looking shrinking behavior would be for this widget, so I'm not going to mess around with it. Go ahead and merge.

@Chaphasilor Chaphasilor merged commit 5924d1c into jmshrv:redesign Apr 1, 2024
2 checks passed
@Chaphasilor
Copy link
Collaborator

Alright, thanks!

If you want to add it but just don't know how it should look, I could mock something up, that wouldn't be a problem :p

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