Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

Feature pull to refresh gallery screen issue [866] #934

Merged
merged 12 commits into from
Dec 27, 2023
Merged

Feature pull to refresh gallery screen issue [866] #934

merged 12 commits into from
Dec 27, 2023

Conversation

sahilsk3333
Copy link
Contributor

Implemented Pull to refresh for gallery screen #866
updated material3 version to "1.2.0-alpha11"

ezgif com-video-to-gif

@sahilsk3333 sahilsk3333 requested a review from a team as a code owner November 26, 2023 11:38
@sahilsk3333 sahilsk3333 changed the title Feature pull to refresh gallery screen issue 866 Feature pull to refresh gallery screen issue [866] Nov 26, 2023
Copy link
Contributor

@arriolac arriolac left a comment

Choose a reason for hiding this comment

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

Few more changes to simplify the code

val photo = pagingItems[index]
"${ photo?.id ?: ""}${index}"

var isRefreshing by remember { mutableStateOf(false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra isRefreshing state is not needed. pullToRefreshState should be the source of truth instead of having 2 different "is refreshing" states.

if (pullToRefreshState.isRefreshing){
LaunchedEffect(Unit){
onPullToRefresh()
isRefreshing = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this per comment above

when (pagingItems.loadState.refresh) {
is LoadState.Loading -> Unit
is LoadState.Error,is LoadState.NotLoading -> {
isRefreshing = false
Copy link
Contributor

Choose a reason for hiding this comment

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

You can call pullToRefreshState.endRefresh() directly here

isRefreshing = true
}
}
LaunchedEffect(isRefreshing){
Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code is not needed

@arriolac
Copy link
Contributor

Looks like the build failed due to a merge conflict. If you can address that on your side I'll go ahead and merge this in. Thanks!

…_866

# Conflicts:
#	app/src/main/java/com/google/samples/apps/sunflower/compose/gallery/GalleryScreen.kt
#	app/src/main/java/com/google/samples/apps/sunflower/compose/home/HomeScreen.kt
…screen_issue_866' into feature_pull_to_refresh_gallery_screen_issue_866

# Conflicts:
#	app/src/main/java/com/google/samples/apps/sunflower/compose/home/HomeScreen.kt
@sahilsk3333
Copy link
Contributor Author

Looks like the build failed due to a merge conflict. If you can address that on your side I'll go ahead and merge this in. Thanks!

Sure i will look into this, Thanks!

Copy link
Contributor

@arriolac arriolac left a comment

Choose a reason for hiding this comment

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

LGTM

@arriolac arriolac merged commit dfa9aea into android:main Dec 27, 2023
2 checks passed
@sahilsk3333
Copy link
Contributor Author

LGTM

Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants