Migrate share fragment to Jetpack Compose #1185
Conversation
| private var disposable: Disposable? = null | ||
|
|
||
| override fun onPause() { | ||
| super.onPause() | ||
|
|
||
| disposable?.dispose() | ||
| } |
There was a problem hiding this comment.
It seems we're not using disposable anywhere. Can we remove the variable and onPause override?
| (dialog as? BottomSheetDialog)?.behavior?.state = BottomSheetBehavior.STATE_EXPANDED | ||
| } | ||
| @Composable | ||
| private fun Pill( |
There was a problem hiding this comment.
There can be reusable components that can be extracted out like this Pill and the component for each option. Can you describe the challenges you had in using the OptionsDialog() that you initially proposed to use?
This will help us decide the architecture for the rest of the "Playback Effects" dialogs.
There was a problem hiding this comment.
I agree, the pill can be reused. Regarding the Options Dialog, it would work but one concern:
If we migrate all fragments using BaseDialogFragment() to optionsDialog(), we would have to add new rows for each specific fragment. That means for each fragment that is being migrated, we would add new custom composables on the dialog. But if this would not be a problem, it would work.
There was a problem hiding this comment.
That means for each fragment that is being migrated, we would add new custom composables on the dialog.
Could we pass a custom option component (which is only shown on a single screen) to the OptionsDialog() so that we don't increase the class size too much and yet be able to have a common class across all these option fragments?
@mchowning, @geekygecko any thoughts?
There was a problem hiding this comment.
@Mzazi25 👋
I had a brief discussion with the team, with OptionsDialog() we'll be able to reuse common components and see some advantages. Can we give it a try?
There was a problem hiding this comment.
Sure thing. Let me work on that
| color = Color.LightGray.copy(0.3f), | ||
| thickness = 1.dp, | ||
| ) | ||
| Divider(thickness = 1.dp) |
There was a problem hiding this comment.
Duplicate divider? There's one above.
|
👋 @Mzazi25 ! Are you still planning to work on this? |
Since we decided to use the option dialog, the only blocker we had is we have to work on the optionDialog configuration first. |
| SourceView.PLAYER, | ||
| analyticsTracker | ||
| ).showShareDialogDirect() | ||
| @Suppress("DEPRECATION") |
There was a problem hiding this comment.
onActivityCreated has been deprecated. Should we consider to use OnViewCreated function? wdyt?
|
|
||
| @Composable | ||
| private fun ShareScreen( | ||
| podcast: Podcast?, |
There was a problem hiding this comment.
As we are passing Podcast and BaseEpisode object to composable function, Should we mark these classes with @stable for compose-runtime?
| onClick = onShareEpisode | ||
| ) | ||
| Divider( | ||
| color = Color.LightGray.copy(0.3f), |
There was a problem hiding this comment.
We should consider to use MaterialTheme outline color as divider colours rather than hardcoding them. Wdyt?
| .fillMaxWidth() | ||
| .clickable( | ||
| interactionSource = remember { MutableInteractionSource() }, | ||
| indication = rememberRipple(color = Color.LightGray) |
There was a problem hiding this comment.
We should consider to use MaterialTheme outline color as divider colours rather than hardcoding them. Wdyt?
Description
This PR migrates the share Fragment views to Jetpack Compose.
The Compose version is the exact replica from the views.
In case of any padding values I missed, I would be glad to counter check
Fixes #
Testing Instructions
Screenshots or Screencast
Checklist
modules/services/localization/src/main/res/values/strings.xmlI have tested any UI changes...