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.

Sign up for GitHub

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

Jump to bottom

[Jetnews] Move NavRail to top level component #655

Merged
merged 5 commits into from
Sep 21, 2021
Merged

[Jetnews] Move NavRail to top level component #655

merged 5 commits into from
Sep 21, 2021

Conversation

manuelvicnt
Copy link
Contributor

No description provided.

@manuelvicnt manuelvicnt changed the base branch from mv/jetnews_topappbar to ls September 15, 2021 17:31
@manuelvicnt manuelvicnt changed the base branch from ls to mv/jetnews_topappbar September 15, 2021 17:32
@manuelvicnt manuelvicnt mentioned this pull request Sep 15, 2021
Merged
alexvanyo
alexvanyo requested changes Sep 15, 2021
View reviewed changes
Copy link
Contributor

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

Leaving a request changes here for the remaining insets issues.

The biggest category that isn't handled right now is when there are start or end system bars, when showing just the list or detail with no nav bar visible. Both the drawer and the content are being obscured by the navigation bar in those cases.

There also looks to be some dark mode issues with the status bar, where the status bar is showing up completely white when in list/detail mode.

Base automatically changed from mv/jetnews_topappbar to ls September 16, 2021 07:18
@manuelvicnt
Copy link
Contributor Author

Hi @alexvanyo! There were a lot of conflicts so I was forced to force push this change. Most problems with insets should be solved. PTAL! Thank you. It's easier to handle insets in this way when all the information is centralised.

alexvanyo
alexvanyo reviewed Sep 17, 2021
View reviewed changes
JetNews/app/src/main/java/com/example/jetnews/ui/JetnewsApp.kt
JetnewsNavGraph(
appContainer = appContainer,
// Either allow showing the drawer, or show the nav rail
showTopAppBar = !showNavRail,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this logic is what we want:

We should show the top app bar when Compact or Medium, since we only want to show the list/detail view with `Expanded.

But here we are only showing the top app bar when Compact, which means that we are showing the list detail view in Medium.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, let's fix this in a different PR. Thanks!

alexvanyo
alexvanyo requested changes Sep 17, 2021
View reviewed changes
Copy link
Contributor

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

Most of the remaining issues are insets related, sorry for the screenshot dump 🙈 .

I think the core logic for when we show the list/detail view also changed, based on how values are passed around.

@JolandaVerhoef
Copy link
Contributor

@alexvanyo Here's a tip for adding screenshots:
After adding the screenshot, copy the file url into the following text:
<img src="" width="200px"/>
This will show your uploaded screenshot in a more manageable size!

@manuelvicnt
Copy link
Contributor Author

Thanks @alexvanyo for your help! The PR is updated with the new changes

alexvanyo
alexvanyo approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

Looks good! We'll have a follow up PR after this one to adjust the Medium behavior (and overall finish up the window size classes)

@manuelvicnt manuelvicnt merged commit 62824b4 into ls Sep 21, 2021
@manuelvicnt manuelvicnt deleted the mv/navrail branch September 21, 2021 07:35
manuelvicnt pushed a commit that referenced this pull request Oct 13, 2021
@alexvanyo @angeles-bilbao6
@FannyDemey @c5inco @JolandaVerhoef @JoseAlcerreca
Adapt Jetnews to large screens and Jetcaster to tabletop posture (#678)
d380475 
* [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 <[email protected]>
Co-authored-by: DEMEY Fanny <[email protected]>
Co-authored-by: Chris Sinco <[email protected]>
Co-authored-by: Jolanda Verhoef <[email protected]>
Co-authored-by: Jose Alcérreca <[email protected]>
Co-authored-by: Alex Vanyo <[email protected]>
Co-authored-by: Nick Rout <[email protected]>

* [Jetcaster] Add tabletop posture support to the Player screen (#676)

Co-authored-by: Alex Vanyo <[email protected]>
Co-authored-by: Angeles <[email protected]>
Co-authored-by: DEMEY Fanny <[email protected]>
Co-authored-by: Chris Sinco <[email protected]>
Co-authored-by: Jolanda Verhoef <[email protected]>
Co-authored-by: Jose Alcérreca <[email protected]>
Co-authored-by: Nick Rout <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JolandaVerhoef JolandaVerhoef JolandaVerhoef approved these changes

@alexvanyo alexvanyo alexvanyo approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@manuelvicnt @JolandaVerhoef @alexvanyo @googlebot