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

Migrate empty_state_view xml/view to Jetpack Compose #11725

Merged

Conversation

Profpatsch
Copy link
Contributor

Followup rebase for #11463 due to technical reasons.

Please see its description.

@github-actions github-actions bot added the size/large PRs with less than 750 changed lines label Nov 20, 2024
@Profpatsch Profpatsch changed the title Lwj.compose migrate empty state view Migrate empty_state_view xml/view to Jetpack Compose Nov 20, 2024
@Profpatsch
Copy link
Contributor Author

A manual test clicking through the build on an empty newpipe seems that everything still works as expected.

@Profpatsch
Copy link
Contributor Author

Profpatsch commented Nov 20, 2024

Some kaomoji that had different variants before seem to be the same now, maybe we want to keep that. Nevermind, that’s handled as well

@TobiGr TobiGr added the rewrite Issues and PRs related to rewrite label Nov 20, 2024
@Profpatsch
Copy link
Contributor Author

Profpatsch commented Nov 20, 2024

I’d merge soon, unless there are any vetoes

@Profpatsch Profpatsch force-pushed the lwj.compose_migrate_empty_state_view branch from 3c0b0be to e05eef0 Compare November 21, 2024 10:52
@Profpatsch Profpatsch force-pushed the lwj.compose_migrate_empty_state_view branch from e05eef0 to ad72b2c Compare November 21, 2024 10:52
@Stypox
Copy link
Member

Stypox commented Nov 21, 2024

Wait

@Stypox
Copy link
Member

Stypox commented Nov 21, 2024

I think that the component's parameters are needlessly complicated. The only three items in the spec should be:

  • modifier: () -> Modifier
  • emojiText: () -> String
  • descriptionText: () -> String

@Stypox
Copy link
Member

Stypox commented Nov 21, 2024

@Stypox
Copy link
Member

Stypox commented Nov 21, 2024

I pushed a few more commits, and now the code is better in my opinion. These things still need to be improved though (I don't know if it's better to do them now or in a future PR):

  • storing a Modifier in the EmptyStateSpec is bad practice and also doesn't make much sense (it's the purpose of parent views to decide the modifiers of the children, it's not a property of the children themselves)
  • the spacing of the UI is not perfect everywhere, but at least with the latest commit the panel is always centered
    • I can't understand why the "Select a playlist" dialog is tight, while "Select a channel" is larger, but I can't debug right now, see below why...
  • (in a future PR!) the error panel should be merged with the empty state panel, since:
    • only one of them is shown at a time
    • the checks for errors are often close to the checks for emptiness, so some checks could be simplified
    • I'm not totally convinced of this though
  • the empty state for unsupported content in the channel fragment does not work, but it didn't work before either, and both my Layout Inspector and by View Designer don't work, so I can't debug right now (Android Studio decided to go on vacation or something)

I tested all the screens above, in dark and light themes, and they work.

@Profpatsch
Copy link
Contributor Author

Looks exceptionally good to me! (disclaimer: I didn’t test, since you already made screenshots)

@Profpatsch Profpatsch merged commit 9b78e49 into TeamNewPipe:refactor Nov 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rewrite Issues and PRs related to rewrite size/large PRs with less than 750 changed lines
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants