-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
refactor: remove arguments from dialog fragment constructors #4684
Conversation
The same would probably also need to be done for everything in |
app/src/main/java/com/github/libretube/ui/dialogs/AddToPlaylistDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/dialogs/DownloadDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/dialogs/PlaylistDescriptionDialog.kt
Outdated
Show resolved
Hide resolved
Please save all the keys used for the bundles inside the |
app/src/main/java/com/github/libretube/ui/fragments/AudioPlayerFragment.kt
Outdated
Show resolved
Hide resolved
Ok. Should I commit from here directly by clicking on 'Commit Suggestion' button or by pushing the changes to the branch? |
It would be better if you do the suggested changes on your PC and push it via git as usual. |
Ok. |
app/src/main/java/com/github/libretube/ui/sheets/PlaylistOptionsBottomSheet.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/sheets/PlaylistOptionsBottomSheet.kt
Outdated
Show resolved
Hide resolved
Thanks for your work on that and applying the suggestions, looks fine to merge from my side when we can get the |
If you want to have a look at the bottom sheets as well: we sometimes pass the player there, the easiest thing probably would be to save the player in the |
app/src/main/java/com/github/libretube/ui/dialogs/AddToPlaylistDialog.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/libretube/ui/dialogs/ShareDialog.kt
Outdated
Show resolved
Hide resolved
Ok. |
app/src/main/java/com/github/libretube/ui/sheets/PlaylistOptionsBottomSheet.kt
Outdated
Show resolved
Hide resolved
A couple of things I want to tell:
|
|
Why status checks aren't running ? |
Probably due to the merge conflicts. |
Everything is done. If there aren't any things need to be corrected from your side, I will push the remaining Dialogs. |
|
Is there something wrong with |
There's a package called |
Ktlint doesn't show anything wrong in IntentData for me. I use windows. |
I have tried placing the |
It asks us to use variable names like |
Sure. Just send me a reply for any changes or to unmark from draft after you have tested. |
If 1. is fixed, it's okay to be merged from my POV. |
|
No, that's clearly not working as it does before the changes. |
We can consider passing the callback functions differently instead of using the val renamePlaylistDialog = RenamePlaylistDialog()
renamePlaylistDialog.onRename = {
// insert the callback function content here
}
renamePlaylistDialog.show(fragmentManager, "foo") where |
Ok. |
Done. |
Any other changes ? |
4560a80
to
061eecf
Compare
- AddToPlaylistDialog - BackupDialog - CreatePlaylistDialog - DeleteAccountDialog - LoginDialog - LogoutDialog
061eecf
to
f260c48
Compare
- ColorPickerDialog - DeletePlaylistDialog - RenamePlaylistDialog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Are you interested in looking into the bottom sheets too, or shall I do that? |
I would but I am kinda busy nowadays due to college. Thanks. |
We're not in a hurry, you can do it whenever you want :) |
Ok. Sounds good. |
For your information, I've already done the changes in some separate PRs now. Nevertheless, feel free to work on any other issue whenever you want to :) |
Ok 👍 |
Addresses #4434