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

Fix artist items continuation #1689

Merged
merged 3 commits into from
Dec 17, 2024
Merged

Conversation

gechoto
Copy link
Contributor

@gechoto gechoto commented Oct 28, 2024

These changes make it possible to load:

  • more than 100 songs on an artists page
  • more than 100 albums on an artists page

Also it removes duplicate code in ArtistItemsContinuationPage.

There is still room for optimization. For example in ArtistItemsScreen we now always call two LaunchedEffect (for lazyListState and lazyGridState).
I don't know how much worse this is for performance.
We could do this conditionally but since I'm still learning and never used Compose before I'm not feeling confident yet doing changes in this part of the code.
Maybe someone else can work on that or I might open a separate draft PR for optimizations.

Also the initial loading placeholders are still not correct. For example when loading albums (displayed in a grid) it still initially shows list item placeholders (because it doesn't know yet that it should display a grid).
Looks a bit weird. But nothing new, this already happened before this PR so I would not change it here. Maybe something for later.

@gechoto
Copy link
Contributor Author

gechoto commented Dec 14, 2024

Hi @z-huang I think this is ready for merge.
I have been using it since I made this PR and haven't found any problems with it.

@z-huang z-huang merged commit bb11093 into z-huang:dev Dec 17, 2024
2 checks passed
@z-huang
Copy link
Owner

z-huang commented Dec 17, 2024

Thanks!

@gechoto gechoto deleted the fix-artistItemsContinuation branch January 24, 2025 12:25
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