-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
base: main
Are you sure you want to change the base?
Conversation
656fd3c
to
b24c039
Compare
Maybe we can wait for #502 to be merged before we start this work? At present, the compose version on the Or create a I'm not sure when |
@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 |
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. |
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 |
Hi, @abcghy Could you please help resolve a code conflict? I think some of our infrastructure is now ready. |
91cf190
to
50b4054
Compare
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. |
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) |
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.
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) |
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.
TODO: handle system navigations & screen orientation changes
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.
@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.
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.
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" 🤞
Okay, please remind me when this PR is ready. |
50b4054
to
f5193c8
Compare
f5193c8
to
8a0eda1
Compare
|
onArticleClick = selectArticle, | ||
) | ||
} | ||
FlowScreenType.ArticleDetails -> { |
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.
The Article -> Flow -> Home
backstack seems broken now
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.
Anyway, we can put this change on hold since even Google haven't agree on the pattern of making a adaptive navigation
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.
The
Article -> Flow -> Home
backstack seems broken now
Could you please elaborate on the work flow?
Finally Google has stabilized their large screen support library! We can begin to leverage References: |
issue: #338
Guideline: https://developer.android.com/jetpack/compose/layouts/adaptive
Official Reference: https://github.com/android/compose-samples/tree/main/JetNews
Design Reference: https://m3.material.io/foundations/layout/canonical-layouts/list-detail