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.

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

[Jetnews] TopAppBar doesn't match design spec #467

Merged
merged 33 commits into from
Aug 30, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d67c992
Issue459 - TopAppBar doesn't match design spec
angeles-bilbao6 Apr 5, 2021
af6c735
Removed unnecessary imports
angeles-bilbao6 Apr 6, 2021
657c99d
Removed wildcard imports
angeles-bilbao6 Apr 6, 2021
5575a95
Removed wildcard imports from interests screen
angeles-bilbao6 Apr 6, 2021
0e2721a
spotless apply
angeles-bilbao6 Apr 6, 2021
a7918ff
Replaced mipmap with a vector asset
angeles-bilbao6 Apr 8, 2021
906b9fd
Merge branch 'main' of https://github.com/android/compose-samples int…
angeles-bilbao6 Apr 13, 2021
57da1df
Fixed files to meet requirements
angeles-bilbao6 Apr 13, 2021
eca2a25
Extract title to string resource
angeles-bilbao6 Apr 13, 2021
70c3597
Changed article icon to png
angeles-bilbao6 Apr 15, 2021
d248739
Set icon as vector asset
angeles-bilbao6 Apr 29, 2021
1dbc9f9
Merge main into branch
angeles-bilbao6 Apr 29, 2021
f9a3b56
Update app_launches test, title is no longer a text
angeles-bilbao6 Apr 29, 2021
6641358
Re base and test changes
angeles-bilbao6 May 10, 2021
f698285
Merge main changes into bug-fix459/top-bar
angeles-bilbao6 May 13, 2021
9e3f1d5
Spotless apply
angeles-bilbao6 May 13, 2021
5569553
Set dark icons if light theme
angeles-bilbao6 May 14, 2021
2e59495
Merge main into bug-fix459/top-bar
angeles-bilbao6 May 14, 2021
9883262
Jetnews fixes
angeles-bilbao6 May 17, 2021
b1e5b2f
Merge main into bug-fix459/top-bar
angeles-bilbao6 May 17, 2021
2d3a680
Spotless apply
angeles-bilbao6 May 17, 2021
aa8e8e5
Modify elevation if scroll event happens
angeles-bilbao6 May 19, 2021
56fdd05
PR Fixes
angeles-bilbao6 Jun 17, 2021
fc3b94b
Merge branch 'main' into bug-fix459/top-bar
angeles-bilbao6 Jun 17, 2021
779ea9b
Merge branch 'main' into bug-fix459/top-bar
angeles-bilbao6 Jul 19, 2021
1242455
Lazy List State adjustments and code polish
angeles-bilbao6 Jul 20, 2021
a093c85
Merge main into bug-fix459/top-bar
angeles-bilbao6 Aug 24, 2021
fb6f61d
PR requested adjustments
angeles-bilbao6 Aug 24, 2021
2041fc2
Spotless kotlin
angeles-bilbao6 Aug 24, 2021
69f5a2c
Update JetNews/app/src/main/java/com/example/jetnews/ui/home/HomeScre…
angeles-bilbao6 Aug 25, 2021
a051cbe
Merge remote-tracking branch 'github/main' into bug-fix459/top-bar2
JoseAlcerreca Aug 27, 2021
6e7528a
Re-adds scrollState after merge
JoseAlcerreca Aug 27, 2021
873bc3c
Merge pull request #1 from JoseAlcerreca/bug-fix459/top-bar2
angeles-bilbao6 Aug 27, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class JetnewsTests {

@Test
fun app_launches() {
composeTestRule.onNodeWithText("Jetnews").assertExists()
composeTestRule.onNodeWithText("Top stories for you").assertExists()
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.example.jetnews.ui

import androidx.compose.material.MaterialTheme
import androidx.compose.material.Scaffold
import androidx.compose.material.rememberScaffoldState
import androidx.compose.runtime.Composable
Expand All @@ -38,8 +39,9 @@ fun JetnewsApp(
JetnewsTheme {
ProvideWindowInsets {
val systemUiController = rememberSystemUiController()
val darkIcons = MaterialTheme.colors.isLight
SideEffect {
systemUiController.setSystemBarsColor(Color.Transparent, darkIcons = false)
systemUiController.setSystemBarsColor(Color.Transparent, darkIcons = darkIcons)
}

val navController = rememberNavController()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,16 @@ package com.example.jetnews.ui.article
import android.content.Context
import android.content.Intent
import android.content.res.Configuration.UI_MODE_NIGHT_YES
import androidx.compose.foundation.Image
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentWidth
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.foundation.shape.CircleShape
import androidx.compose.material.AlertDialog
import androidx.compose.material.Icon
import androidx.compose.material.IconButton
Expand All @@ -46,6 +51,7 @@ import androidx.compose.runtime.saveable.rememberSaveable
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
Expand Down Expand Up @@ -127,25 +133,45 @@ fun ArticleScreen(
if (showDialog) {
FunctionalityNotAvailablePopup { showDialog = false }
}

val scrollState = rememberLazyListState()
Scaffold(
topBar = {
InsetAwareTopAppBar(
title = {
Text(
text = "Published in: ${post.publication?.name}",
style = MaterialTheme.typography.subtitle2,
color = LocalContentColor.current
)
Row(
modifier = Modifier
.fillMaxWidth()
.wrapContentWidth(align = Alignment.CenterHorizontally)
.padding(start = 30.dp)
) {
Image(
painter = painterResource(id = R.drawable.icon_article_background),
contentDescription = null,
modifier = Modifier
.clip(CircleShape)
.size(36.dp)
)
Text(
text = stringResource(id = R.string.published_in, post.publication?.name ?: ""),
style = MaterialTheme.typography.subtitle2,
color = LocalContentColor.current,
modifier = Modifier
.padding(start = 10.dp)
.weight(1.5f)
)
}
},
navigationIcon = {
IconButton(onClick = onBack) {
Icon(
imageVector = Icons.Filled.ArrowBack,
contentDescription = stringResource(R.string.cd_navigate_up)
contentDescription = stringResource(R.string.cd_navigate_up),
tint = MaterialTheme.colors.primary
)
}
}
},
elevation = if (scrollState.firstVisibleItemScrollOffset == 0) 0.dp else 4.dp,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename the file "LazyListUtils.kt" as it's not holding any state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

backgroundColor = MaterialTheme.colors.surface
)
},
bottomBar = {
Expand All @@ -159,6 +185,7 @@ fun ArticleScreen(
) { innerPadding ->
PostContent(
post = post,
state = scrollState,
modifier = Modifier
// innerPadding takes into account the top and bottom bar
.padding(innerPadding)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.LazyListState
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.shape.CircleShape
import androidx.compose.material.Colors
Expand Down Expand Up @@ -78,8 +79,9 @@ import com.example.jetnews.ui.theme.JetnewsTheme
private val defaultSpacerSize = 16.dp

@Composable
fun PostContent(post: Post, modifier: Modifier = Modifier) {
fun PostContent(post: Post, state: LazyListState, modifier: Modifier = Modifier) {
JoseAlcerreca marked this conversation as resolved.
Show resolved Hide resolved
LazyColumn(
state = state,
modifier = modifier.padding(horizontal = defaultSpacerSize)
) {
item {
Expand Down Expand Up @@ -349,7 +351,7 @@ private val Colors.codeBlockBackground: Color
fun PreviewPost() {
JetnewsTheme {
Surface {
PostContent(post = post3)
PostContent(post = post3, LazyListState())
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,18 @@
package com.example.jetnews.ui.home

import android.content.res.Configuration.UI_MODE_NIGHT_YES
import androidx.compose.foundation.Image
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.wrapContentSize
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.LazyListState
import androidx.compose.foundation.lazy.LazyRow
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.material.CircularProgressIndicator
import androidx.compose.material.Divider
import androidx.compose.material.ExperimentalMaterialApi
Expand All @@ -37,6 +40,8 @@ import androidx.compose.material.ScaffoldState
import androidx.compose.material.SnackbarResult
import androidx.compose.material.Text
import androidx.compose.material.TextButton
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.Search
import androidx.compose.material.rememberScaffoldState
import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
Expand All @@ -46,6 +51,7 @@ import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.ColorFilter
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.style.TextAlign
Expand Down Expand Up @@ -162,33 +168,58 @@ fun HomeScreen(
}
}
}

val scrollState = rememberLazyListState()
Scaffold(
scaffoldState = scaffoldState,
topBar = {
val title = stringResource(id = R.string.app_name)
InsetAwareTopAppBar(
title = { Text(text = title) },
title = {
Icon(
painter = painterResource(R.drawable.ic_jetnews_wordmark),
contentDescription = title,
tint = MaterialTheme.colors.onBackground,
modifier = Modifier
.fillMaxSize()
.padding(bottom = 4.dp, top = 10.dp)
)
},
navigationIcon = {
IconButton(onClick = { coroutineScope.launch { openDrawer() } }) {
Icon(
Image(
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use an Icon here and set the tint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see Image?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, please replace with icon

From IconButton:

content should typically be an Icon, using an icon from androidx.compose.material.icons.Icons. If using a custom icon, note that the typical size for the internal icon is 24 x 24 dp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just made the change

painter = painterResource(R.drawable.ic_jetnews_logo),
contentDescription = stringResource(R.string.cd_open_navigation_drawer)
contentDescription = stringResource(R.string.cd_open_navigation_drawer),
colorFilter = ColorFilter.tint(MaterialTheme.colors.primary)
)
}
}
},
actions = {
IconButton(
onClick = { /* TODO: Open search */ }
) {
Icon(
imageVector = Icons.Filled.Search,
contentDescription = stringResource(R.string.cd_search)
)
}
},
backgroundColor = MaterialTheme.colors.surface,
elevation = if (scrollState.firstVisibleItemScrollOffset == 0) 0.dp else 4.dp
JoseAlcerreca marked this conversation as resolved.
Show resolved Hide resolved
)
}
) { innerPadding ->

val modifier = Modifier.padding(innerPadding)
LoadingContent(
empty = posts.initialLoad,
emptyContent = { FullScreenLoading() },
loading = posts.loading,
onRefresh = onRefreshPosts,
content = {

HomeScreenErrorAndContent(
posts = posts,
state = scrollState,
onRefresh = {
onRefreshPosts()
},
Expand Down Expand Up @@ -243,14 +274,15 @@ private fun LoadingContent(
@Composable
private fun HomeScreenErrorAndContent(
posts: UiState<List<Post>>,
state: LazyListState,
onRefresh: () -> Unit,
navigateToArticle: (String) -> Unit,
favorites: Set<String>,
onToggleFavorite: (String) -> Unit,
modifier: Modifier = Modifier
) {
if (posts.data != null) {
PostList(posts.data, navigateToArticle, favorites, onToggleFavorite, modifier)
PostList(posts.data, state, navigateToArticle, favorites, onToggleFavorite, modifier)
} else if (!posts.hasError) {
// if there are no posts, and no error, let the user refresh manually
TextButton(onClick = onRefresh, modifier.fillMaxSize()) {
Expand All @@ -275,6 +307,7 @@ private fun HomeScreenErrorAndContent(
@Composable
private fun PostList(
posts: List<Post>,
state: LazyListState,
JoseAlcerreca marked this conversation as resolved.
Show resolved Hide resolved
navigateToArticle: (postId: String) -> Unit,
favorites: Set<String>,
onToggleFavorite: (String) -> Unit,
Expand All @@ -287,6 +320,7 @@ private fun PostList(

LazyColumn(
modifier = modifier,
state = state,
contentPadding = rememberInsetsPaddingValues(
insets = LocalWindowInsets.current.systemBars,
applyTop = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.heightIn
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.paddingFromBaseline
Expand All @@ -40,6 +41,8 @@ import androidx.compose.material.Surface
import androidx.compose.material.Tab
import androidx.compose.material.TabRow
import androidx.compose.material.Text
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.Search
import androidx.compose.material.rememberScaffoldState
import androidx.compose.runtime.Composable
import androidx.compose.runtime.collectAsState
Expand All @@ -54,6 +57,7 @@ import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.heading
import androidx.compose.ui.semantics.semantics
import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.tooling.preview.Devices
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
Expand Down Expand Up @@ -178,16 +182,36 @@ fun InterestsScreen(
Scaffold(
scaffoldState = scaffoldState,
topBar = {

InsetAwareTopAppBar(
title = { Text("Interests") },
title = {
Text(
text = stringResource(R.string.cd_interests),
modifier = Modifier.fillMaxWidth(),
textAlign = TextAlign.Center
)
},
navigationIcon = {
IconButton(onClick = openDrawer) {
Icon(
painter = painterResource(R.drawable.ic_jetnews_logo),
contentDescription = stringResource(R.string.cd_open_navigation_drawer)
contentDescription = stringResource(R.string.cd_open_navigation_drawer),
tint = MaterialTheme.colors.primary
)
}
}
},
actions = {
IconButton(
onClick = { /* TODO: Open search */ }
) {
Icon(
imageVector = Icons.Filled.Search,
contentDescription = stringResource(R.string.cd_search)
)
}
},
backgroundColor = MaterialTheme.colors.surface,
elevation = 0.dp
JoseAlcerreca marked this conversation as resolved.
Show resolved Hide resolved
)
}
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ private val LightThemeColors = lightColors(
secondary = Red700,
secondaryVariant = Red900,
onSecondary = Color.White,
error = Red800
error = Red800,
onBackground = Color.Black,

)

private val DarkThemeColors = darkColors(
Expand All @@ -39,7 +41,8 @@ private val DarkThemeColors = darkColors(
onPrimary = Color.Black,
secondary = Red300,
onSecondary = Color.Black,
error = Red200
error = Red200,
onBackground = Color.White
)

@Composable
Expand Down
5 changes: 5 additions & 0 deletions JetNews/app/src/main/res/drawable/ic_article_icon.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<vector android:height="24dp" android:tint="#3DDC84"
JoseAlcerreca marked this conversation as resolved.
Show resolved Hide resolved
android:viewportHeight="24" android:viewportWidth="24"
android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android">
<path android:fillColor="#FF000000" android:pathData="M17.6,11.48 L19.44,8.3a0.63,0.63 0,0 0,-1.09 -0.63l-1.88,3.24a11.43,11.43 0,0 0,-8.94 0L5.65,7.67a0.63,0.63 0,0 0,-1.09 0.63L6.4,11.48A10.81,10.81 0,0 0,1 20L23,20A10.81,10.81 0,0 0,17.6 11.48ZM7,17.25A1.25,1.25 0,1 1,8.25 16,1.25 1.25,0 0,1 7,17.25ZM17,17.25A1.25,1.25 0,1 1,18.25 16,1.25 1.25,0 0,1 17,17.25Z"/>
</vector>
14 changes: 14 additions & 0 deletions JetNews/app/src/main/res/drawable/icon_article_background.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android"
JoseAlcerreca marked this conversation as resolved.
Show resolved Hide resolved
android:width="48dp"
android:height="48dp"
android:viewportWidth="24.0"
android:viewportHeight="24.0">
<group>
<path android:name="square"
android:fillColor="#FF073042"
android:pathData="M0,0 L24,0 L24,24 L0,24 z" />
<path android:fillColor="#3DDC84"
android:pathData="M17.6,11.48 L19.44,8.3a0.63,0.63 0,0 0,-1.09 -0.63l-1.88,3.24a11.43,11.43 0,0 0,-8.94 0L5.65,7.67a0.63,0.63 0,0 0,-1.09 0.63L6.4,11.48A10.81,10.81 0,0 0,1 20L23,20A10.81,10.81 0,0 0,17.6 11.48ZM7,17.25A1.25,1.25 0,1 1,8.25 16,1.25 1.25,0 0,1 7,17.25ZM17,17.25A1.25,1.25 0,1 1,18.25 16,1.25 1.25,0 0,1 17,17.25Z"/>

</group>
</vector>
3 changes: 3 additions & 0 deletions JetNews/app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
<string name="cd_bookmark">Bookmark</string>
<string name="unbookmark">unbookmark</string>
<string name="bookmark">bookmark</string>
<string name="cd_search">Search</string>
<string name="cd_interests">Interests</string>
<string name="published_in">Published in: \n%1$s</string>
<string name="fewer_stories">Show fewer stories like this?</string>
<string name="fewer_stories_content">This feature is not yet implemented</string>
<string name="agree">AGREE</string>
Expand Down