-
-
Notifications
You must be signed in to change notification settings - Fork 691
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
Compose About Activity conversion #5246
base: main
Are you sure you want to change the base?
Conversation
- adds HtmlText component - adds preference for testing
- adds proper html string - adds body and footer for the about Activity - updates Snackbar, HtmlText - fixes app theme issue, - ui fixes
- code fixes - converts LicenseActivity to Compose
# Conflicts: # app/src/main/AndroidManifest.xml
), | ||
linkInteractionListener = { | ||
if (linkTextData.asset != null) { | ||
println("orange: content ${linkTextData.text}") |
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.
Please remove test related println()
code.
}, | ||
topBar = { | ||
WikiTopAppBar( | ||
title = "About", |
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.
Please use the string reference.
) | ||
}, | ||
topBar = { | ||
WikiTopAppBar( |
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.
Something I have noticed is that it does not show the tooltip for the back button. Would it be possible to update the WikiTopAppBar
for the back button (home button) for its content description for accessibility?
} | ||
|
||
@Composable | ||
fun AboutScreenContent( |
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 like the way you separate the layout into different sections!
}, | ||
), | ||
painter = painterResource(R.drawable.w_nav_mark), | ||
contentDescription = null, |
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.
Since this is a clickable icon, to improve accessibility, consider adding a new string for the content description of the globe logo.
private lateinit var binding: ActivityAboutBinding | ||
@Composable | ||
fun AboutWikipediaScreen( | ||
modifier: Modifier = Modifier, |
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.
It looks like an unused parameter, can we remove it?
|
||
@Preview | ||
@Composable | ||
private fun AboutWikipediaImagePreview() { |
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 sure but do we need to keep this one?
binding = ActivityLicenseBinding.inflate(layoutInflater) | ||
setContentView(binding.root) | ||
setNavigationBarColor(getThemedColor(this, android.R.attr.windowBackground)) | ||
val asset = intent.getStringExtra(ASSET) ?: "" |
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.
It is fine to use the "asset" string here.
What does this do?
Converts About Activity to Compose
Phabricator:
https://phabricator.wikimedia.org/T384362
This also resolves the license screen issue.
https://phabricator.wikimedia.org/T384526