-
Notifications
You must be signed in to change notification settings - Fork 44
SES-4433 : Handle landscape mode #1527
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
base: dev
Are you sure you want to change the base?
SES-4433 : Handle landscape mode #1527
Conversation
This reverts commit 20a5fa9.
Here are a few issues from testing the branch:
|
modifier = Modifier.fillMaxWidth().padding(24.dp), | ||
contentAlignment = Alignment.Center | ||
) { | ||
Text(text = stringResource(messageId), style = MaterialTheme.typography.bodyMedium) |
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.
We don't reference material's typography in the app, but instead use our own type system.
Instead of MaterialTheme.typography.bodyMedium
this should use LocalType.current.large
(because large
matches the 16sp that were used in the xml file).
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.
Updated.
@Composable | ||
private fun GiphyLoading() { | ||
Box( | ||
modifier = Modifier.fillMaxWidth().padding(vertical = 24.dp), |
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.
For common spacing we should defer to our Dimensions file. so instead of hardcoding 24.dp here (and in line 42) you can use LocalDimensions.current.spacing
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.
Updated.
}).attach(); | ||
// new TabLayoutMediator(binding.tabLayout, binding.giphyPager, (tab, position) -> { | ||
// tab.setText(position == 0 ? NonTranslatableStringConstants.GIF : getString(R.string.stickers)); | ||
// }).attach(); |
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.
This can be deleted if we are no longer using it
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.
Updated.
selectedIndex: Int, | ||
onSelect: (Int) -> Unit | ||
) { | ||
val indicatorColor = colorFromAttrOr( |
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.
We shouldn't rely on material themes directly in the app.
We already have a SessionTabRow
that is styled as per the rest of the app, so it should be used here.
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.
Updated.
} | ||
|
||
@Composable | ||
private fun colorFromAttrOr(@AttrRes attrResId: Int, fallback: Color): Color { |
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.
This likely can be deleted as we already have a styled tab component, SessionTabRow
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.
Updated.
ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed | ||
) | ||
|
||
composeView.setContent { |
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.
Not a huge deal but this doesn't really match our existing format when it comes to compose content.
You could have the lines above directly in the activity.
And the lines below here could be in the GiphyScreen composable (with the file renamed too), matching the way we handle compose content in other screens.
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.
Got it. I might have to convert the activity here to Kotlin before I change this.
import androidx.compose.ui.platform.LocalConfiguration | ||
|
||
object AdaptiveBreakpoints { | ||
const val TWO_PANE_LANDSCAPE_MIN_WIDTH_DP = 480 |
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.
This class is full of specific values and mentions of two pane, which is a specific UI paradigm we are not yet handling, like showing two panes: one for the conversation list and the other for the conversation screen itself, all in the same window.
I don't think we should reinvent the wheel here, and instead we should rely on existing material values
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.
I removed the initial two pane content here and kept the landscape for now. I'll add to this helper in the future when we decide to cater for multiple screen sizes.
} | ||
|
||
@Composable | ||
private fun TwoPaneContent(string: String) { |
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.
We have to be careful with naming things like this as two panes UI is a specific thing, which we will need to handle in the future, with the material adaptive library. But these are two panes, but more like a landscape version of a UI.
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.
Updated to specify Landscape and Portrait.
} | ||
|
||
@Composable | ||
private fun TwoPaneContent( |
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.
Same here. We need to be careful with concept like two pane which is a material adaptive concept that is a bit different. Here you could simply have a portrait and a landscape version of the composable.
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.
Updated to specify Landscape and Portrait.
modifier = Modifier.verticalScroll(rememberScrollState()) | ||
) { | ||
ActionList(navigateTo = navigateTo) | ||
Column( |
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.
Isn't there a way for both landscape and portrait to reuse some element instead of redeclaring them here?
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.
Updated, I made the QrPanel reusable.
import network.loki.messenger.R | ||
|
||
/** Called from Java to set Compose content for the loading view. */ | ||
fun setGiphyLoading(composeView: ComposeView) { |
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.
I'm not sure that is is very efficient. Setting the overall content of the screen and resetting the strategy each time.
The whole content should exist in the one composable: the loading, the 'no result' and the list of results.
If not then maybe it was better off in the initial xml format, otherwise it becomes conceptually broken?
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.
I updated this one to use a state now instead of redeclaring and resetting it every time. I think that made it more efficient?
Updates on this:
|
This PR removes portrait lock.
This PR also adds some new composables for different orientations.
TODO: Full end to end testing for landscape.