Skip to content
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

Android13 changes #38

Merged
merged 36 commits into from Jun 27, 2022
Merged

Android13 changes #38

merged 36 commits into from Jun 27, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 15, 2022

  • new google API to handle in-app language changes
  • simple backup configuration
  • new savedstatehandle to get/set StateFlow's from it
  • ability to test changes with fragments
  • ability to pick 1/multiple images from gallery
  • 2 helper methods to observe changes, made by other fragments, and to get savedhandle to make this changes
  • use hiltViewModel() method instead of "by viewModels" helper method in compose fragment

Ivan Kashtelyan and others added 24 commits May 20, 2022 17:23
…introduce few helper "observe" methods, use new savedstatehandle apis, improve fragments to get rid of "by viewModels" extensions in compose fragments
@ghost ghost requested a review from Skyyo June 15, 2022 15:37
Copy link
Owner

@Skyyo Skyyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. pushed few changes since new google API to handle in-app language changes was misused. Though there is an issue on Android versions lower than Android 13 - activity isn't restarted automatically. Not sure if that's intended, or will be changed when Android 13 is released. If this issue persists - we will need to add a TODO into the readme here and to check on it later
  2. ability to test changes with fragments - can you please elaborate on this one? Didn't understand what to check.
  3. 2 helper methods to observe changes, made by other fragments, and to get savedhandle to make this changes - let's please try to apply them in the package named navigateWithResult, if it simplifies things.
  4. use hiltViewModel() method instead of "by viewModels" helper method in compose fragment - can you please remind me if we can drop dependency or is there any noticable benefit of doing so? Seems it makes more sense to use the current approach until we ditch Fragments completely

Good job overall @ahamula !

@ghost
Copy link
Author

ghost commented Jun 20, 2022

  1. Activity do restarts, but it restores previous state afterwards. Please, add logs in onCreate method to verify it. I don't know if other projects have same requirements as Puff, but on Puff there is requirement to turn hebrew as default locale, so we need to use activity instead of fragment/composable, because language screen not necessary will be on top. On Puff there is some guidance screen on top, and so we can't apply default language logic in fragment.
  2. There was flag USE_BOTTOM_NAVIGATION_WITH_FRAGMENTS in MainActivity, that you was able to switch on/off to verify that language change work with fragments fine. Also we were able to test some other features with fragments, not fully composable's screens just to turn on this flag.
  3. I'm talking about these 2 methods: fun NavigationDispatcher.observeBackStackStateHandle() and fun NavigationDispatcher.observeNavigationResult() in NavControllerExtensions, not clear where you'd like them to put.
  4. We can drop "by viewModels" extension, and instead use hiltViewModel() method inside composable. Not much noticeable benefits, just we can remove one method, and don't have long "non-composable" methods (I mean onCreateView will become in 4-5 lines long). You can check code of some fragment from application.fragment package. So, no much benefit, but we can simplify migration from fragment to composable in future (if it'll happen :) ) Because it's all copy paste, only "main composable" part differs between fragments

@Skyyo

@ghost ghost requested a review from Skyyo June 20, 2022 06:42
Copy link
Owner

@Skyyo Skyyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 - in case activity restarts, we need to ensure that everything else knows that locale changed. We want every Android version to react immediately, like on the Android 13 atm.
2 - no need or fragments in this project. Didn't delete them yet, since there is still usage of your 2 extensions.
3 - here. These screens showcase passing fields up & down
4 - got it, won't be using it for now.

@ghost
Copy link
Author

ghost commented Jun 20, 2022

@Skyyo Fixed navigation in "dog"/"cat" places, fixed layout direction on android 12

@ghost ghost requested a review from Skyyo June 20, 2022 09:48
@ghost ghost marked this pull request as draft June 24, 2022 16:17
# Conflicts:
#	app/build.gradle
#	app/src/main/AndroidManifest.xml
@ghost ghost marked this pull request as ready for review June 27, 2022 08:35
Andrii Hamula and others added 7 commits June 27, 2022 19:26
# Conflicts:
#	app/build.gradle
#	app/src/main/java/com/skyyo/samples/application/Destination.kt
#	app/src/main/java/com/skyyo/samples/application/activity/PopulatedNavHost.kt
#	app/src/main/java/com/skyyo/samples/features/sampleContainer/SampleContainerScreen.kt
#	app/src/main/java/com/skyyo/samples/features/sampleContainer/SampleContainerViewModel.kt
@Skyyo Skyyo merged commit ddbb9b3 into master Jun 27, 2022
@Skyyo Skyyo deleted the android13_changes branch June 27, 2022 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant