MediaSend update 1 : Moved fragment UI to compose #4
Conversation
| ) { | ||
|
|
||
| // span logic: screenWidth / media_picker_folder_width | ||
| val folderWidth = dimensionResource(R.dimen.media_picker_folder_width) |
There was a problem hiding this comment.
You probably want to port this into our compose Dimension.kt instead. You can remove media_picker_folder_width and other related xml dimensions if they are not needed or moved to the kt file instead.
| modifier = Modifier | ||
| .align(Alignment.BottomCenter) | ||
| .fillMaxWidth() | ||
| .height(50.dp) |
There was a problem hiding this comment.
Throughout this file I see a lot of hardcoded dp values: In paddings, width, size, height. These should be taken from our Dimensions file.
And if the values don't exist in Dimensions you have to ask yourself whether we really need a new value or whether you can reuse existing values.
The old designs are full of inconsistencies so the idea if not necessarily to redo it exactly how it was, but by moving to Compose we have to see if we can normalise the layouts to use values we use in the rest of the app for consistency.
Some component might require unique/special values, and for these, especially if it's unique and never used elsewhere, then you can hardcode the dp value in the compose directly, but for a lot of element we have to think about consistency first, and see if we can make things look cohesive with the rest of the app by taking values form Dimensions.kt
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
One thing to be mindful of if you decide to keep all the old stuff, is to not forget to clean it out later.
With git history you can easily access old files you should be ok deleting and cleaning everything you don't need anymore.
Don't forget to clean out the layout.xml files, and possibly unused views.xml, dimensions, etc...
| .clickable(onClick = onClick) | ||
| ) { | ||
| Box(modifier = Modifier.aspectRatio(1f)) { | ||
| GlideImage( |
There was a problem hiding this comment.
We are moving away from Glide so you should no longer use these.
Instead use the Coil components, like AsyncImage
| modifier = Modifier.fillMaxSize(), | ||
| contentScale = ContentScale.Crop | ||
| ) | ||
| // Bottom shade overlay |
There was a problem hiding this comment.
Do you think it could be achieved more easily with the innerShadow modifier?
| selected.indexOfFirst { it.uri == media.uri } | ||
| } | ||
|
|
||
| // Matches adapter rules: |
There was a problem hiding this comment.
We have to be mindful of not re-implementing old bad behaviours.
Since we are moving things to Compose, it has to be done cleanly.
Composables should be pretty dumb, so if there some state management to be done, it should happen in the VM.
You might not need exactly the same values and properties that were in the old code, instead you have to think: What does this specific component need, then only give it that, and work back from there to find whose responsibility it is to provide this data. It likely will be the VM.
| .padding(end = 2.dp, bottom = 2.dp) | ||
| .aspectRatio(1f) | ||
| .combinedClickable( | ||
| onClick = { |
There was a problem hiding this comment.
Logic in a composable is suspicious.
Again, we shouldn't match old behaviour, we need to cleanly move to compose by doing the hard work of moving the responsibility to the right place, likely the VM.
|
|
||
| Box( | ||
| modifier = modifier | ||
| .padding(end = 2.dp, bottom = 2.dp) |
There was a problem hiding this comment.
Is it ok to including padding this way, shouldn't it be the responsibility of the parent who uses these items to space them out properly?
What if you end up reusing these components somewhere else? The composables should be pretty simple and the calling sites can decide on how to space them
| ) | ||
|
|
||
| // Border overlay (replaces @drawable/mediapicker_item_border_dark View) | ||
| Box( |
There was a problem hiding this comment.
You don't need extra boxes when applying things like a border. You can apply the modifier to the composable in question directly.
| painter = painterResource(R.drawable.triangle_right), | ||
| contentDescription = null, | ||
| modifier = Modifier | ||
| .size(width = 15.dp, height = 18.dp) |
There was a problem hiding this comment.
As mentioned there are a lot of hardcoded sizes everywhere here. We need to make sure we don't just copy the old xml code but also try to normalise and standardise the new compose components to match the more recent parts of the app, so we can try to use sizes from the Dimensions.kt file
|
|
||
| val context = LocalContext.current | ||
| val activity = context as? FragmentActivity | ||
| val showManage = remember(activity) { |
There was a problem hiding this comment.
Suspicious of logic managed in a composable. It should get its state handed to it.
This is tied to a specific activity. I wonder how this is going to work once you go to full compose.
AttachmentManager might no longer be the right file for this logic? We need to rethink the format now that we are moving things to a modern structure. Should this be VM logic?
| .fillMaxSize() | ||
| .background(LocalColors.current.background) | ||
| ) { | ||
| items(folders) { folder -> |
There was a problem hiding this comment.
It's good to add a key for items
Moved the old Fragment XMLs to Compose.
New ComposeFragments are created to replace the old fragments for the MediaSendActivity.
Part 2 in progress : MediaSendFragment UI to compose.