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

feat: add color tokens #152

Draft
wants to merge 33 commits into
base: develop
Choose a base branch
from
Draft

feat: add color tokens #152

wants to merge 33 commits into from

Conversation

paulinea
Copy link
Member

No description provided.

Copy link

github-actions bot commented Oct 24, 2024

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

I've modified the parser to generate the OudsColor*SemanticTokens.kt.
I've been able to compare the content of this PR's files and the generated ones.
Great job doing that semi-manually! I've found out only 2 differences that you can find in the comments.

val gradientSkeletonStartEndLight: Color = ColorRawTokens.colorFunctionalLightGray80,
val gradientSkeletonStartEndOnBgEmphasizedLight: Color = ColorRawTokens.colorFunctionalDarkGray720,
val gradientSkeletonMiddleDark: Color = ColorRawTokens.colorFunctionalDarkGray720,
val gradientSkeletonMiddleOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray560,
Copy link
Member

Choose a reason for hiding this comment

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

From the parser:

Suggested change
val gradientSkeletonMiddleOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray560,
val gradientSkeletonMiddleOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray480,

val gradientSkeletonMiddleDark: Color = ColorRawTokens.colorFunctionalDarkGray720,
val gradientSkeletonMiddleOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray560,
val gradientSkeletonStartEndDark: Color = ColorRawTokens.colorFunctionalDarkGray800,
val gradientSkeletonStartEndOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray640,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val gradientSkeletonStartEndOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray640,
val gradientSkeletonStartEndOnBgEmphasizedDark: Color = ColorRawTokens.colorFunctionalDarkGray560,

Copy link
Member

@julien-deramond julien-deramond Oct 28, 2024

Choose a reason for hiding this comment

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

The brand-theme.orange_light.json defines the colors; in the structure of this JSON file, we have:

{
  // ...
  color: {
    action: { //... },
    always: { //... },
    bg: { //... },
    border: { //... },
    brand: { //... },
    chart: { //... },
    content: { //... },
    elevation: { //... },
    gradient: { //... },
    transparent: { //... },
    🎨_decorative: { //... },
  }
  // ...
}
  • chart is removed temporarily
  • 🎨_decorative will be OudsColorDecorativeSemanticTokens
  • For all other keys, we'll uppercase the name to have OudsColor{Key}SemanticTokens

Based on these elements, I'd say that OudsColorGlobalSemanticTokens.kt should rather be OudsColorTransparentSemanticTokens.kt shouldn't it?

val modifier = when (style) {
OudsBorderStyle.None -> Modifier
OudsBorderStyle.Solid -> Modifier.border(width = width, color = borderColor, shape = shape)
OudsBorderStyle.Dashed -> Modifier.dashedBorder(width = width, color = borderColor, shape = shape)
OudsBorderStyle.Dotted -> Modifier.dottedBorder(width = width, color = borderColor, shape = shape)
}
IllustrationBox(modifier = modifier, backgroundColor = Color.Transparent)
IllustrationBox(modifier = modifier, backgroundColor = OudsColorKeyToken.Background.Secondary.value)
Copy link
Member

Choose a reason for hiding this comment

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

There is an issue with the pill radius background because it is not clipped to the border:

Screenshot_20241030_154034

Here is the fix:

Suggested change
IllustrationBox(modifier = modifier, backgroundColor = OudsColorKeyToken.Background.Secondary.value)
IllustrationBox(modifier = modifier.clip(shape), backgroundColor = OudsColorKeyToken.Background.Secondary.value)

val disabledContentColor: OudsColorKeyToken = OudsColorKeyToken.OnSecondary,
val containerColor: OudsColorKeyToken.Background = OudsColorKeyToken.Background.BrandPrimary,
val contentColor: OudsColorKeyToken.Content = OudsColorKeyToken.Content.BrandPrimary,
val disabledContainerColor: OudsColorKeyToken.Action = OudsColorKeyToken.Action.Disabled,
Copy link
Member

Choose a reason for hiding this comment

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

Type is OudsColorKeyToken.Action, which means we cannot set this value to OudsColorKeyToken.Content.Disabled for instance. It seems to me that we should be able to use any semantic color token, am I wrong?

import com.orange.ouds.tokens.global.raw.ColorRawTokens

data class OudsColorGradientSemanticTokens(
val gradientSkeletonMiddleLight: Color = ColorRawTokens.colorFunctionalLightGray160,
Copy link
Member

Choose a reason for hiding this comment

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

I certainly missed something, but why inverse colors do not appear here?


package com.orange.ouds.theme.tokens

class OudsColorKeyToken {
Copy link
Member

Choose a reason for hiding this comment

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

This class could be an object.

TransparentDefault,
}

enum class Action {
Copy link
Member

Choose a reason for hiding this comment

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

I already added a comment in #147 about adding nested classes to split enum values. This comment is just to remember to apply the changes here if we decide to do so for other tokens.

val transparentDefault: Color
)

data class Action(
Copy link
Member

Choose a reason for hiding this comment

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

Should we split negative, primary and secondary parameters into specific objects?

Same question for all nested classes.


@Stable
fun OudsColorScheme.fromToken(token: OudsColorKeyToken.Always): Color {
return when (token) {
Copy link
Member

Choose a reason for hiding this comment

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

We can use with(alwaysColors) to simplify the following lines.

Same comment for other fromToken methods.

}

val OudsColorSemanticTokens.materialLightColorScheme: ColorScheme
get() = lightColorScheme(
Copy link
Member

Choose a reason for hiding this comment

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

Are these final mapping from Maxime? Or is this temporary?

secondary = backgroundColorTokens.bgSecondaryDark,
onSecondary = elevationColorTokens.elevationDragOnBgSecondaryDark,
secondaryContainer = backgroundColorTokens.bgEmphasizedDark,
onSecondaryContainer = contentColorTokens.contentDefaultOnBgEmphasizedDark,
Copy link
Member

Choose a reason for hiding this comment

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

Light value for onSecondaryContainer is contentDefaultDark, should we use contentDefaultLight here?

Same question for onErrorContainer where contentOnStatusNegativeEmphasizedDark is used although the light version is contentOnActionDisabledOnBgEmphasizedLight.

/**
* Converts an OUDS decorative color token to the local color value provided by the theme.
*/
val OudsColorKeyToken.Decorative.value: Color
Copy link
Member

Choose a reason for hiding this comment

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

value method for OudsColorKeyToken.Global is missing.

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

Successfully merging this pull request may close these issues.

Tokens: Colors
3 participants