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

Custom theming #77

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Custom theming #77

wants to merge 16 commits into from

Conversation

ninovanhooff
Copy link
Collaborator

@ninovanhooff ninovanhooff commented Sep 10, 2024

Why is this important?

  • It was on the wishlist (in readme)
  • MaterialTheme is nice, but it cannot be reconciled with iOS. Therefore tokens in a shared design system are always necessary

Notes

  • Inspired by Hema
  • Removes dynamic color. Can probably be incorporated somewhere, but definetlely not 90% of projects use it
  • explicitly setting font style and color for all texts seems tedious, but in practice the boilerplate is reduced by creating core components like buttons etc (see TemplateButton())

because that is what we are implementing here
Inspired by Hema implementation
@ninovanhooff ninovanhooff added the enhancement New feature or request label Sep 10, 2024
Copy link
Collaborator

@izadiegizabal izadiegizabal left a comment

Choose a reason for hiding this comment

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

nice seeing some more theming added to the template app! i left a couple of comments 🪄

core/ui/src/main/kotlin/nl/q42/template/ui/theme/Theme.kt Outdated Show resolved Hide resolved
Comment on lines 42 to 45
SideEffect {
val window = (view.context as Activity).window
window.statusBarColor = colorScheme.primary.toArgb()
window.statusBarColor = colors.accent.toArgb()
WindowCompat.getInsetsController(window, view).isAppearanceLightStatusBars = darkTheme
Copy link
Collaborator

Choose a reason for hiding this comment

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

core/ui/src/main/kotlin/nl/q42/template/ui/theme/Theme.kt Outdated Show resolved Hide resolved
core/ui/src/main/kotlin/nl/q42/template/ui/theme/Dimens.kt Outdated Show resolved Hide resolved
Copy link
Collaborator

@izadiegizabal izadiegizabal left a comment

Choose a reason for hiding this comment

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

a couple of minor improvements but nice work!

Comment on lines +8 to +10
override val textPrimary = Black
override val surface = White
override val surfaceSelected = Color(0xff1b1b1b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

its a very small nitpic but if we already started making the type explciit for the others, i would expect it to have it for all

Comment on lines +45 to +52
val view = LocalView.current
if (!view.isInEditMode) {
SideEffect {
val window = (view.context as Activity).window
window.statusBarColor = colors.accent.toArgb()
WindowCompat.getInsetsController(window, view).isAppearanceLightStatusBars = darkTheme
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this for?

typography: AppTypography = AppTheme.typography,
colors: AppColorScheme = AppTheme.colors,
shapes: AppShapes = AppTheme.shapes,

Copy link
Collaborator

Choose a reason for hiding this comment

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

there shouldn't be a new line here

import nl.q42.template.ui.theme.PreviewLightDark

@Composable
fun TemplateButton(text: String, enabled: Boolean = true, onClick: () -> Unit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the naming differentiation between AppXYZ and TemplateXYZ things?

val White = Color(0xFFFFFFFF)
val Gray10 = Color(0xFF1b1b1b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we have a EOF new line?

val componentSpacingVertical = 24.dp
/** Spacing between 2 vertically stacked buttons */
val buttonSpacingVertical = 8.dp
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we have a EOF new line?

Comment on lines +46 to +47
style = AppTheme.typography.body,
color = AppTheme.colors.error
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we want to have this as a showcase screen on how to do things, the style & color here should be abstracted into a themed text component that directly use as the regular material Text (similar to the buttons)

@@ -40,10 +40,8 @@ fun HomeSecondScreen(
verticalArrangement = Arrangement.Center,
) {

Text(viewState.title, style = MaterialTheme.typography.titleMedium)
Text(viewState.title, style = AppTheme.typography.h1, color = AppTheme.colors.textPrimary)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants