- Notifications
You must be signed in to change notification settings - Fork 5.3k
[JetNews] List-detail support for main page #629
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
Conversation
7f688ff to d671872 Compare | All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
| All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
40cb601 to 7ce5d37 Compare
manuelvicnt left a comment
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.
Left some initial comments, some of them are due to the lack of familiarity with the navigation flow you have in mind. We should try to work on simplifying it as the implementation detail looks tedious. Ideally, someone without the flow chart should be able to follow along easily.
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt Outdated Show resolved Hide resolved
242d982 to b2cf96b Compare | CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
| CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| @manuelvicnt I just pushed up another commit, which makes quite a few changes. Most notably, I've removed the article route entirely (for now), so articles are now only displayed from the I also created a |
b2cf96b to c0c5c03 Compare | All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
| All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeRoute.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeRoute.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeRoute.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeRoute.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreens.kt Outdated Show resolved Hide resolved
| CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| @manuelvicnt This is ready for another look! I've fully rebased on to the navrail PR, and have fixed up a couple things related to that rebase. I also got to all of the comments you left, and added quite a bit of extra inline documentation around the approach. |
manuelvicnt left a comment
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.
Two nits but LGTM overall. Thank you!
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeRoute.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreens.kt Outdated Show resolved Hide resolved
| Also @alexvanyo, can you add screenshots to the description of the PR for reviewers to get a sense of what's been implemented? At some point, we'll also have those mocks in the README. Thanks! |
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeRoute.kt Outdated Show resolved Hide resolved
| Is there a reason that you included the migration from |
b72caae to 1df7d7f Compare | @JolandaVerhoef Ah I apologize, that ended up being a refactor that happened partway through while cleaning everything up. Ideally that should have been pulled out into a separate PR that didn't touch the UI at all. That ended up being changed to more nicely support having the topmost article selected by default. Previously, the post list was expecting a hardcoded list that was pulled apart into different sections, so the 4th post was arbitrarily chosen to be the "highlighted item." With the refactor to |
| Thanks @manuelvicnt and @JolandaVerhoef ! |
Adds an initial list-detail support for the main page.
The overall approach relies on distinguishing between what the user sees as different screens, and what we implement as different navigation routes.
Navigation-wise, the "home" route is responsible for drawing the list detail screen, as well as the list screen and detail screen on their own (for small devices). Distinguishing between which one to show is now simply a matter of keeping track of whether the user is looking at the list, or a selected article.
Doing so allows meeting a couple of nice behaviors: