The Wayback Machine - https://web.archive.org/web/20201019190248/https://github.com/android/compose-samples/pull/116
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

Refactor JetNews to use unidirectional data flow #116

Closed
wants to merge 21 commits into from
Closed

Conversation

@objcode
Copy link
Collaborator

@objcode objcode commented Aug 19, 2020

Several large refactors to land this:

  • Removed RefreshableUiState
  • Reworked UiState to allow data, error, and loading at the same time (to support all use cases)
  • Replaced effects with coroutine-based [launchUiStateProducer]
  • Refactored Repository to expose Flow and suspend functions throughout
  • Deleted JetNewsStatus global state
  • Used savedInstanceState where appropriate to support rotation

Composables of screens were refactored to follow hoisting best practices

  • Each screen has a single stateful composable paired with a stateless composable at the top level
  • Stateful composables do all state management
  • Added slots where appropriate to avoid too many parameters

ArticleScreen:

  • Moved favorites to repository from JetNewsStatus
  • Refactored composables to use new state

HomeScreen:

  • Renames to make composables names match their function
  • Added slot to LoadingContent to extract a composable focused on initial load and swipe to refresh
  • Renamed HomeScreenBodyWrapper to HomeScreenErrorAndContent to better match function
  • Added empty UI state if the initial load fails ("Tap to load content")
  • Hoisted snackbar state [UiState] and made HomeScreenErrorAndContent stateless
  • Renamed all composables related to displaying the post list to PostList from HomeScreenBody etc.
  • Used unidirectional data flow throughout post list composables
  • Renamed LoadingHomeScreen to FullScreenLoading to better match function
  • Animated ErrorSnackbar on entrance and exit using AnimatedVisibility

InterestsScreen:

  • Introduced TabContent object to describe a tab and it's composable content, this allows for slot usage for screen-level composables
  • Stateless InterestScreen now takes a List which describes the tabs and their content
  • Renamed InterestScreenBody to TabContent to better match function
  • Moved selected topics to repository, and removed JetNewsStatus
  • Renamed *Tab composables to *List composables (e.g. TopicList) to better match function
  • Removed string-key from topics, and introduced TopicSelection for TabWithSections
  • Moved all previously global state into Repository (which is a singleton)
  • Use unidirectional data flow throughout this screen

SwipeToRefreshLayout:

  • Added TODO to document a know bug (b/164113834) that this code works around
  • Disable swiping when state.value == true to avoid a bug created by duplicated state showing an infinite loading spinner
Several large refactors to land this:

- Removed RefreshableUiState
- Reworked UiState to allow data, error, and loading at the same time (to support all use cases)
- Removed effects architecture and restructured using ViewModel
- Refactored Repository to expose Flow and suspend functions throughout
- Deleted JetNewsStatus global state

Composables of screens were refactored to follow hoisting best practices
- Each screen has a single stateful composable paired with a stateless composable at the top level
- Stateful composabels use ViewModel to manage state
- Added slots where appropriate to avoid too many parameters

ArticleScreen:
- Moved favorites to ViewModel from JetNewsStatus
- Refactored composables to use new state

HomeScreen:
- Renames to make composables names match their function
- Added slot to LoadingContnent to extract a composable focused on initial load and swipe to refresh
- Renamed HomeScreenBodyWrapper to HomeScreenErrorAndContent to better match function
- Added empty UI state if the initial load fails ("Tap to load content")
- Hoisted snackbar state to ViewModel and made HomeScreenErrorAndContent stateless
- Renamed all composables related to displaying the post list to PostList from HomeScreenBody etc.
- Used unidirectional data flow throughout post list composables
- Renamed LoadingHomeScreen to FullScreenLoading to better match function
- Animated ErrorSnackbar on entrance and exit using AnimatedVisibility

InterestsScreen:
- Introduced TabContent object to describe a tab and it's composable content, this allows for slot usage for screen-level composables
- Stateless InterestScreen now takes a List<TabContent> which describes the tabs and their content
- Renamed InterestScreenBody to TabContent to better match function
- Moved selected topics to ViewModel, and removed global state
- Renamed *Tab composables to *List composables (e.g. TopicList) to better match function
- Removed string-key from topics, and introduced TopicSelection for TabWithSections
- Moved all previously global state into Repository (which is a singleton)
- Use unidirectional data flow throughout this screen

SwipeToRefreshLayout:
- Added TODO to document a know bug (b/164113834) that this code works around
- Disable swiping when state.value == true to avoid a bug created by duplicated state showing an infinite loading spinner
@googlebot googlebot added the cla: yes label Aug 19, 2020
@objcode objcode requested a review from manuelvicnt Aug 19, 2020
@JoseAlcerreca JoseAlcerreca self-requested a review Aug 19, 2020
Copy link
Collaborator

@JoseAlcerreca JoseAlcerreca left a comment

Stopship! I think scoping ArticleViewModel to fragment/activity is preventing the postId from being updated after the initial load
(factory = ArticleViewModelFactory(postId, postsRepository)

objcode added 4 commits Aug 19, 2020
…l with the previous postId.
- prefer observe* name for Flow returning functions in Repository
- Make Set<T>.addOrRemove an internal extension function and use it throughout
- Deprecate sample implementation of ViewModelLifecycleScope as it is intended to be replaced with a public API
- Add missing comments to ArticleViewModel.kt
- Corrected comments about viewModel scope to say Activity instead of Application
- Added MapExtensions.kt that contains an internal extension addOrRemove
- Swapped to use the liveData builder for ArticleViewModel.post
… and re-order parameters to match stateless.
…nstances on rotation.
@objcode
Copy link
Collaborator Author

@objcode objcode commented Aug 19, 2020

Thanks for reviews! Updated based on comments. Please take a look again.

…effect similar to the previous architecture.

Re-add ViewModel when https://issuetracker.google.com/issues/165642391 is fixed

Changes:
- Created launchUiStateProducer effect to safely call suspending data producers from compose and return refreshable UI state
- Removed ViewModel from HomeScreen, InterestsScreen and ArticleScreen
- Refactored stateful screen composables to use the new loading effect
@objcode objcode changed the title Add ViewModel to JetNews. Refactor JetNews to use unidirectional data flow Aug 20, 2020
objcode added 2 commits Aug 20, 2020
…ent, return State<T>, and use a Channel instead of a ConflatedBroadcastChannel.
Copy link
Collaborator

@chrisbanes chrisbanes left a comment

LGTM

objcode added 4 commits Aug 21, 2020
@objcode objcode requested a review from manuelvicnt Aug 21, 2020
@objcode objcode requested a review from JoseAlcerreca Aug 21, 2020
@objcode objcode requested a review from JoseAlcerreca Aug 21, 2020
@objcode
Copy link
Collaborator Author

@objcode objcode commented Aug 21, 2020

Manually merged to develop w/ squashes.

@objcode objcode closed this Aug 21, 2020
@chrisbanes chrisbanes deleted the viewModel branch Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.