-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
First steps in Jetpack Compose (reimplement parts of the user screen) #5607
Conversation
This will only work if there is only dark theme and light theme, but nothing else is planned
but sadly without the "pop out" so far (not sure how to do this)
…pe into own composable
Is it possible to have an APK to test these changes? |
I also changed some behavior (slightly) because it was easier to do in Compose while others were mere complicated to do. @Jean-BaptisteC Why would you want to test the changes? It should work as before, except the some small things I changed. And it is just three screens. |
import org.koin.androidx.viewmodel.ext.android.viewModel | ||
|
||
/** activity only used in debug, to show all achievement links */ | ||
class ShowLinksActivity : BaseActivity() { |
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.
@FloEdelmann I removed this because it is more convenient to preview the links directly in the IDE, see LazyLinksColumn.kt
@@ -75,37 +68,6 @@ class ProfileViewModelImpl( | |||
override val biggestSolvedCountCountryStatistics = MutableStateFlow<CountryStatistics?>(null) | |||
override val biggestSolvedCountCurrentWeekCountryStatistics = MutableStateFlow<CountryStatistics?>(null) | |||
|
|||
override var lastShownGlobalUserRank: Int? |
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.
@matkoniecz I removed this because I found it to be a bit complicated or at least not worth the amount of code to keep it in Compose. Now, the badges just always animate, which is nicer anyway, IMO.
Sure, the sense of progression is lost, but that feature was too well hidden anyway, i.e. it was barely noticeable what changed exactly because the animation was played automatically on entering the screen, so one would have to look really quick anyway to see what the value before was.
And making it more prominent would on the other hand usually just look too disruptive. (Don't need a "New achievement unlocked"-type of dialog every time my rank is changed towards the better)
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.
After encountering it several times since them I want to say - this was actually a significant improvement in how nice this menu is!
import de.westnordost.streetcomplete.ui.theme.LeafGreen | ||
|
||
@Composable | ||
fun LaurelWreath( |
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.
@matkoniecz I re-did the laurel wreath in compose. Feel free to look through the code to understand what I changed.
Changes to current behavior:
|
Nobody interested to look through this? Well, I suppose if interested, you can also still look through it when it has been merged. Maybe before or after merging this, I will look into if I can replicate something like described here - https://fvilarino.medium.com/shared-element-transitions-in-jetpack-compose-8f553078101e - in order to re-enable this:
|
# Conflicts: # app/src/main/java/de/westnordost/streetcomplete/screens/user/UserActivity.kt # app/src/main/res/layout/fragment_quest_answer.xml
Did you try the screen in dark mode? |
Hmm... I thought I did. I will check. |
I re-implemented the UI of parts of the user screen in compose and set up compose in general (define the default theme etc.). When this is merged, others can contribute re-implementations of parts of the UI in compose.
I initially planned to re-implement the whole user screen, but I came upon two issues which made me postpone this. (Click for more info)
physics-ballpit needs to be replaced with something more boring. On the slim chance that someone with know-how pops up or when I return back to this, I have become a Compose-wizard, I'll leave this open for now (Replace ballpit quest-view in user profile with something else #5595)
Login currently uses the WebView, for which there is no official Compose replacement. There is a library by a third party, but it misses at least one crucial part of the API to be used for OAuth authorization -
shouldOverrideUrlLoading
- used for retrieving the authorization code from the callback URL. So, maybe it is better to use the browser for OAuth authorization instead of a webview (which is also recommended in OAuth 2 anyway) but in any case I will postpone this because of that.@ Contributors and people thinking about contributing in the future: Feel free to look through this and learn from it.
To be honest, I have to admit that it's quite fun to use Compose, the API, at least to do the standard things, is super straightforward. The previewing tools are excellent. But since the design is being done in code, it leaves more opportunities to turn the layout code into unreadable blurb, which is why more care is necessary splitting up components in the UI into own reusable composables for readability (and reusability). After all, this wasn't really done at all before, as all design was locked away in the layout XMLs (with lots of duplication inside). One reason for the almost -1000 lines of code on this PR.
@ People who know Jetpack Compose: Feel free to look through this and suggest improvements.
Since having viewmodels is almost something like a requirement to effectively convert the UI to compose and sometimes it is a bit more complex, I will continue with that after this PR is merged.
Fixes #5580.