[Jetnews] Add NavRail for large screens #636
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
alexvanyo reviewed Aug 30, 2021
alexvanyo reviewed Aug 31, 2021
alexvanyo reviewed Aug 31, 2021
JetNews/app/src/main/java/com/example/jetnews/ui/interests/InterestsScreen.kt Outdated Show resolved Hide resolved
alexvanyo reviewed Aug 31, 2021
| screenshot LGTM - one nit not related to nav rail - the "bookmark" icon seem to be a different color (on the nav rail they should all be "high emphasis") |
alexvanyo reviewed Aug 31, 2021
JetNews/app/src/main/java/com/example/jetnews/ui/components/AppNavRail.kt Show resolved Hide resolved
alexvanyo reviewed Aug 31, 2021
JetNews/app/src/main/java/com/example/jetnews/ui/article/ArticleScreen.kt Outdated Show resolved Hide resolved
alexvanyo requested changes Sep 1, 2021
Contributor
alexvanyo left a comment
alexvanyo left a comment There was a problem hiding this comment.
Two small issues here:
- In dark mode, the elevation of the toolbar doesn't quite match the elevation of the nav rail, which looks a little strange. I wonder if we want to have those match?
- When there's a vertical navigation bar, the padding isn't being handled properly with the nav bar (it looks like this is also an existing issue for the drawer)
JetNews/app/src/main/java/com/example/jetnews/ui/article/ArticleScreen.kt Show resolved Hide resolved
Contributor Author
| Thanks @alexvanyo ! Fixed the issue with the elevation in dark mode. For the other one, we should probably create issues in the repo in case someone wants to fix them before we can spend time on it. The other navigation issue might be worth adding there as well. |
alexvanyo approved these changes Sep 1, 2021
Contributor
alexvanyo left a comment
alexvanyo left a comment There was a problem hiding this comment.
@manuelvicnt Sounds good! I'm going to have to take the insets into account for the list/detail view, so I will probably be able to handle it there when adding in the NavRail as well.
This looks good to me to merge, to keep progress going!
JoseAlcerreca requested changes Sep 2, 2021
Contributor
JoseAlcerreca left a comment
JoseAlcerreca left a comment There was a problem hiding this comment.
I'm seeing rendering issues on the Freeform emulator but I think it's because of the emulator. Can you confirm nothing weird happens when you click on one of the interests?
JetNews/app/src/main/java/com/example/jetnews/ui/article/ArticleScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/article/ArticleScreen.kt 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/interests/InterestsScreen.kt Outdated Show resolved Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/interests/InterestsScreen.kt Show resolved Hide resolved
JoseAlcerreca approved these changes Sep 2, 2021
manuelvicnt pushed a commit that referenced this pull request Oct 13, 2021
* [Jetnews] Snackbars large screens support (#631) * [Jetnews] Add WindowSize breakpoints (#632) * [Jetnews] Add NavRail for large screens (#636) * Add initial large screen exploration * PR feedback and refine integration with navrail * Fix formatting and tests * Use rememberUpdatedState for notifyInput * Add remember for windowSize * [Jetnews] Interests screen LS support (#639) * [Jetnews] Large screen support for the Interests tab (#641) * [JetNews] Fix state saving with drawer (#653) * [JetNews] Fix state saving with drawer * Extract into rememberSizeAwareDrawerState * [JetNews] Convert navrail/drawer decision to use currentWindowMetrics (#654) * [JetNews] Convert navrail/drawer decision to use currentWindowMetrics * Move window metrics helpers to WindowSize.kt * [Jetnews] Better TopAppBar in large screens (#650) * [Jetnews] Move NavRail to top level component (#655) * [JetNews] Update window size breakpoints (#658) * [JetNews] Adjust medium behavior (#657) * [JetNews] Adjust medium behavior * Move column decision out of BoxWithConstraints * Follow my own rule for isDrawerActive * Update for PR feedback * Spotless apply * Add TODO for BoxWithConstraints usage * [JetNews] Replace currentWindowMetrics flow with synchronous calculation (#660) * [Jetnews] Consolidate compact and medium layouts (#659) * [Jetnews] Add space between NavRail icons (#663) * [Jetnews] Custom interests layout (#665) * [JetNews] Move window size calculation to activity level (#667) * [JetNews] Move window size calculation to activity level * Re-order WindowSize functions * Fix formatting * Merge main into ls (#672) * Implemented swipe to delete for Jetsnack cart * Replace logic for SwipeToDismiss composable Added Animated visibility as well * Requested adjustments * Removed Opt in * Spotless apply * PR fixes * Swipe to delete polish * Spotless * Removed unnecessary Row * Code polish * feat : improve UX by setting Button semantic role to play image button fix : fix not beeing able to move from CategoryPodcast to EpisodeList with Talkback feat: merge podcast category card with podcast title * Break long lines * Content to trailing lambda, added comments * Fix invalid neutral color #591 * [Jetnews] Showcase PreviewParameterProvider usage. * [Jetnews] Move PostPreviewParameterProvider.kt and add documentation * Fix kdoc format * [Jetsnack] Snackbar support (#645) * Finish checksum.sh sentence (#622) * [All] Update to Compose 1.0.2 * Spotless apply * [All] Revert to previous version of ktlint * [Jetsnack] Add state holder to JetsnackApp (#646) * fix : fixing formating issue * Update Compose Material Catalog AOSP link (#656) * [Crane] Use createEmptyComposeRule for test with injection * [All] Update to Compose 1.0.3 (#671) * [Jetnews] Update to Compose 1.1.0-alpha05 * [Jetnews] Spotless Co-authored-by: Angeles <angeles.bilbao6@gmail.com> Co-authored-by: DEMEY Fanny <fanny.pluvinage@gmail.com> Co-authored-by: Chris Sinco <csinco@google.com> Co-authored-by: Jolanda Verhoef <JolandaVerhoef@users.noreply.github.com> Co-authored-by: Jose Alcérreca <JoseAlcerreca@users.noreply.github.com> Co-authored-by: Alex Vanyo <vanyo@google.com> Co-authored-by: Nick Rout <nickrout@google.com> * [Jetcaster] Add tabletop posture support to the Player screen (#676) Co-authored-by: Alex Vanyo <vanyo@google.com> Co-authored-by: Angeles <angeles.bilbao6@gmail.com> Co-authored-by: DEMEY Fanny <fanny.pluvinage@gmail.com> Co-authored-by: Chris Sinco <csinco@google.com> Co-authored-by: Jolanda Verhoef <JolandaVerhoef@users.noreply.github.com> Co-authored-by: Jose Alcérreca <JoseAlcerreca@users.noreply.github.com> Co-authored-by: Nick Rout <nickrout@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Add
NavRailto the different screens in Jetnews. When theNavRailappears, theDrawerand possibleBottomBarlayouts disappear. This happens for foldable and tablet layouts.