[Jetnews] TopAppBar doesn't match design spec #467
Conversation
Added corrections for the top Bar in order to look like the most recent design
…o bug-fix459/top-bar
angeles-bilbao6
changed the title
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
Outdated
Show resolved
Hide resolved
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/interests/InterestsScreen.kt
Outdated
Show resolved
Hide resolved
Removed unnecessary column, add colors to the Material Theme and change text to null
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/theme/Theme.kt
Outdated
Show resolved
Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/theme/Theme.kt
Outdated
Show resolved
Hide resolved
|
There are some conflicts, probably because of the work on Window Insets. Can you rebase and verify your changes work with both navigation types (3-button vs gesture), in landscape mode, in dark mode, and on large screens? |
|
Can you please add screenshots of the dark mode / landscape / 3 button nav / large screen use cases? I'd love to see if the app behaves correctly in all of those cases! |
|
|
|
@angeles-bilbao6 I think there's an issue with the light theme, where the status bar icons are not visible. They should show up as dark icons when the theme is set to light. Otherwise, this looks good! |
|
Hi @JolandaVerhoef have you had the chance to review the latest changes for this PR? |
| ) | ||
| } | ||
| } | ||
| }, | ||
| elevation = if (scrollState.firstVisibleItemScrollOffset == 0) 0.dp else 4.dp, |
There was a problem hiding this comment.
I think the intention here is to increase the elevation if the list has been scrolled? I don't think this code is sufficcient i.e. if you scroll in a new item but it's at 0 offset the elevation will go back to 0.dp. A neat way to fix this might be an ext fun like:
val LazyListState.isScrolled: Boolean
get() = firstVisibleItemIndex > 0 || firstVisibleItemScrollOffset > 0
There was a problem hiding this comment.
Please rename the file "LazyListUtils.kt" as it's not holding any state.
JetNews/app/src/main/java/com/example/jetnews/ui/article/PostContent.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
|
Thanks for the changes, a few more comments and then I think we can land this. |
angeles-bilbao6
force-pushed
the
bug-fix459/top-bar
branch
from
e8c2587 to
779ea9b
Compare
|
Some emulators on GHA are acting up so if 2/3 emulator runs pass, consider the PR green please. |
|
Hi @nickbutcher I have already addressed your latest comments, let me know if you are ok with the fixes. |
|
Hi @nickbutcher have you had the chance to review the latest changes? |
|
Went through all the comments resolving some and commented on the ones that were not addressed. |
angeles-bilbao6
force-pushed
the
bug-fix459/top-bar
branch
from
b6a0519 to
a093c85
Compare
|
Hi @JoseAlcerreca I just addressed your last comments |
JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
|
Hi @angeles-bilbao6 , @manuelvicnt merged a big change to JetNews so there was a big conflict. This PR should have been merged yesterday so I resolved it for you in angeles-bilbao6#1 If you merge it should update this PR, I hope :) |
Merges latest changes into android#467's branch
|
Hi @JoseAlcerreca I merged your changes and everything works as expected! |
|
Woohoo, thanks @angeles-bilbao6! |
Added corrections for the TopAppBar in order to look like the most recent design
#459
Home
Before
After
Article
Before
After
Interests
Before
After
