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

large screen support (WIP) #560

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

@abcghy abcghy marked this pull request as draft January 23, 2024 07:28
@JunkFood02
Copy link
Collaborator

@Ashinch
Copy link
Owner

Ashinch commented Jan 23, 2024

Maybe we can wait for #502 to be merged before we start this work?

At present, the compose version on the main branch is far behind.

Or create a dev branch for experimental functions based on #502.

I'm not sure when 0.9.12 will be released. There are still some problems with Google Reader API integration. I'm worried that #502 will wait too long, so that the new features will not be start.

@mbestavros
Copy link
Collaborator

@Ashinch If delaying @JunkFood02's Compose update (#502) is holding community development back, why not merge it now? Especially if Greader needs a bit more time to cook as well.

@Ashinch
Copy link
Owner

Ashinch commented Jan 30, 2024

@Ashinch If delaying @JunkFood02's Compose update (#502) is holding community development back, why not merge it now? Especially if Greader needs a bit more time to cook as well.

Hi, @mbestavros

Google Reader API does indeed need some improvement time. Do you have any suggestions? For example, creating a new dev branch to merge these PRs, allowing the new PRs to continue development using the dev branch,after stabilizing, merge the dev branch back into the main branch? 🤔

@Ashinch
Copy link
Owner

Ashinch commented Jan 30, 2024

@Ashinch If delaying @JunkFood02's Compose update (#502) is holding community development back, why not merge it now? Especially if Greader needs a bit more time to cook as well.

Hi, @mbestavros

Google Reader API does indeed need some improvement time. Do you have any suggestions? For example, creating a new dev branch to merge these PRs, allowing the new PRs to continue development using the dev branch,after stabilizing, merge the dev branch back into the main branch? 🤔

The reason for blocking #502 is that it involves a significant dependency upgrade, and I'm not sure if the UI will work properly. Currently, I'm dealing with issues related to the Google Reader API, so I might not have time to migrate the changes brought by #502.

@JunkFood02
Copy link
Collaborator

JunkFood02 commented Jan 30, 2024

not sure if the UI will work properly

Have been using the app with latest Compose for a while (not much!), and I didn't see any noticeable glitch or bug in it. To be honest, since stable release (v1.0) is still far ahead, users should be more open to help test potential issues as long as there're still updates and fixes

@Ashinch Ashinch changed the base branch from main to dev January 30, 2024 17:27
@Ashinch Ashinch marked this pull request as ready for review January 30, 2024 17:28
@JunkFood02 JunkFood02 self-assigned this Jan 31, 2024
@Ashinch Ashinch changed the base branch from dev to main February 6, 2024 17:37
@Ashinch
Copy link
Owner

Ashinch commented Feb 22, 2024

Hi, @abcghy

Could you please help resolve a code conflict? I think some of our infrastructure is now ready.

@Ashinch Ashinch added this to the 0.9.13 milestone Feb 22, 2024
@Ashinch
Copy link
Owner

Ashinch commented Mar 8, 2024

Hi, everyone.

What is the current status of this PR?

Has anyone else tried it yet?

I want to know if it can be merged. ☺️

@JunkFood02
Copy link
Collaborator

JunkFood02 commented Mar 8, 2024

I've just reviewed this implementation before. It's a rather simple walkaround for large screen support, but lack of handling many corner cases, like rotating the screen or system navigation. Anyway, existing users(compact screen width) shouldn't be affected by this change, so I'm approving it

@@ -82,7 +86,7 @@ class MainActivity : AppCompatActivity() {
removeOnNewIntentListener(listener)
}
}
HomeEntry(subscribeViewModel = subscribeViewModel)
HomeEntry(subscribeViewModel = subscribeViewModel, widthSizeClass = widthSizeClass)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use CompositionLocal instead of a explicit parameter seems neater

)
}
forwardAndBackwardComposable(route = "${RouteName.READING}/{articleId}") {
ReadingPage(navController = navController, homeViewModel = homeViewModel)
ReadingPage(navController = navController, homeViewModel = homeViewModel, isExpandedScreen = isExpandedScreen)
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: handle system navigations & screen orientation changes

Copy link
Author

Choose a reason for hiding this comment

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

@JunkFood02 Could you elaborate on this one? Currently I only added a FlowRoute so that it can react to different width, that include the screen orientation changes.

Copy link
Collaborator

@JunkFood02 JunkFood02 Mar 28, 2024

Choose a reason for hiding this comment

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

Sure! The FlowWithArticleDetailsScreen consists of two destination: a flow view and an article reader screen.

When rotating from expanded to compact width with an article open, the user will be taken back to the flow screen instead of switching to a full-screen article reader. This also happens in landscape mode when trying to navigate back from the article reader.

This issue is addressed in the blog post of Compose team, maybe you can get some inspirations at the part of "Preserving the user’s state through screen size changes" 🤞

@Ashinch
Copy link
Owner

Ashinch commented Mar 8, 2024

Okay, please remind me when this PR is ready.

@abcghy
Copy link
Author

abcghy commented Mar 29, 2024

  1. added isArticleOpen and articleId in HomeUiState.
  2. Combine Reading Page with Flow Page into FlowRoute. So that we can determine which page to show according to screen width and isArticleOpen.
  3. Removed Reading Page from navigation due the reason above.

@JunkFood02

@JunkFood02 JunkFood02 self-requested a review March 29, 2024 17:48
onArticleClick = selectArticle,
)
}
FlowScreenType.ArticleDetails -> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Article -> Flow -> Home backstack seems broken now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, we can put this change on hold since even Google haven't agree on the pattern of making a adaptive navigation

Copy link
Author

Choose a reason for hiding this comment

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

The Article -> Flow -> Home backstack seems broken now

Could you please elaborate on the work flow?

@Ashinch Ashinch modified the milestones: 0.10.0, 1.0.0 Jun 2, 2024
@JunkFood02
Copy link
Collaborator

JunkFood02 commented Jun 3, 2024

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.

4 participants