Improve previews #46
Improve previews #46
Conversation
|
Ahoy! It looks like this PR is a bit behind develop, sorry about not merging it earlier. Happy to accept the change if you'd like to update it to the current develop branch - otherwise will close this on Mar 30 (and feel free to reopen)! Thanks, |
|
Hey @objcode sure thing, will pick it up over the next few days and rebase on master |
Also fixes a bunch of missing tintings so that Dark themes are 100% legible
rock3r
force-pushed
the
improve_previews
branch
from
034d20b
to
72eb831
Compare
|
@objcode all yours. Snuck in a bunch of other fixes too, while I was at that |
| @@ -103,6 +106,12 @@ fun AppDrawer( | |||
| } | |||
| } | |||
|
|
|||
| private fun dividerColor(isLight: Boolean) = if (isLight) { | |||
There was a problem hiding this comment.
Couldn't this be an alpha modification of a theme color like onSurface?
There was a problem hiding this comment.
I admit my ignorance on that, will check if I can do the same. I went with an approach of "keep the colour where it was already defined" but will see if I can use a theme colour instead.
JetNews/app/src/main/java/com/example/jetnews/ui/PreviewUtils.kt
Outdated
Show resolved
Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/article/ArticleScreen.kt
Outdated
Show resolved
Hide resolved
| VectorImage(id = R.drawable.ic_account_circle_black) | ||
| VectorImage( | ||
| id = R.drawable.ic_account_circle_black, | ||
| tint = MaterialTheme.colors().onSurface |
There was a problem hiding this comment.
Should the tint be contentColor() here?
There was a problem hiding this comment.
Not sure what the design intent was originally this so I went for final result. Happy to change.
| @@ -311,8 +329,24 @@ fun Markup.toAnnotatedStringItem(typography: Typography): AnnotatedString.Item<S | |||
| } | |||
| } | |||
|
|
|||
| @Preview | |||
| private fun codeBlockBackground(isLight: Boolean) = if (isLight) { | |||
There was a problem hiding this comment.
Again, couldn't this be an alpha mod of onSurface?
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/PostCards.kt
Outdated
Show resolved
Hide resolved
Co-Authored-By: Nick Butcher <nickbutcher@users.noreply.github.com>
…leScreen.kt Co-Authored-By: Nick Butcher <nickbutcher@users.noreply.github.com>
…ontent.kt Co-Authored-By: Nick Butcher <nickbutcher@users.noreply.github.com>
…s.kt Co-Authored-By: Nick Butcher <nickbutcher@users.noreply.github.com>
|
PR went red after applying suggestions... will review asap and fix :) |
|
All done and ready to go |
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/PostContent.kt
Outdated
Show resolved
Hide resolved
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/PostCardTop.kt
Outdated
Show resolved
Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/PostCardTop.kt
Outdated
Show resolved
Hide resolved
JetNews/app/src/main/java/com/example/jetnews/ui/home/PostCards.kt
Outdated
Show resolved
Hide resolved
|
Thanks for the changes, sorry just spotted a few more. |
|
No worries — I did fix some inconsistencies in the naming I spotted in the codebase but clearly forgot some :) Push incoming |
|
|
|
Thanks for the changes! All LGTM but looks like needs a rebase. |
|
@nickbutcher oh yeah, not sure why it says there's no conflicts here... |
|
Hmm, now it let me merge?! Oh well. Thanks again! |
|
No worries :) |
This PR improves previews for composable functions, by: