Fix/ loadDesignTokens API #2302
Conversation
src/style/colors.ts
Outdated
| @@ -49,10 +49,10 @@ export class Colors { | |||
| * Load light and dark schemes based on generated design tokens | |||
| * @param color - palette color | |||
| */ | |||
| loadDesignTokens(color: string) { | |||
| loadDesignTokens({primaryColor}: {primaryColor: string}) { | |||
There was a problem hiding this comment.
Why not make the type a defined type, like export type TokensOptions: {primaryColor: string}, and have a parameter like loadDesignTokens(options: TokensOptions)? Then you can use options?.primaryColor etc.
src/style/colors.ts
Outdated
|
|
||
| private generateDarkModeTokens(color: string) { | ||
| const colorPalette: string[] = _.reverse(this.generatePalette(color)); | ||
| private generateDesignTokens({primaryColor, dark}: {primaryColor: string; dark?: boolean}) { |
There was a problem hiding this comment.
Why the parameter here is an object? This method is private so no worries about changing its signature in the future. The parameters could be simply primaryColor: string and dark: boolean.
src/style/colors.ts
Outdated
| const colorPalette: string[] = _.reverse(this.generatePalette(color)); | ||
| private generateDesignTokens({primaryColor, dark}: {primaryColor: string; dark?: boolean}) { | ||
| const colorPalette: string[] = dark | ||
| ? _.reverse(this.generatePalette(primaryColor)) |
There was a problem hiding this comment.
You can generate the palette outside of the condition and reverse the array if it is for a dark palette, this way you save the code duplication
There was a problem hiding this comment.
I actually tried it and found it less readable that generateDesignTokens() will need to get the primaryColor, the palette, and the dark boolean as parameters. it gets all the logic out of the method.
And I think it's not that bad to call it twice since it happens only once when the app starts.
There was a problem hiding this comment.
I agree that the palette should be generated in the method, but you can still do it once and only reverse the array if dark = true. For example:
let colorPalette = this.generatePalette(primaryColor); if (dark) { colorPalette = _.reverse(colorPalette); }
src/style/colors.ts
Outdated
| const color30 = colorPalette[2]; | ||
| const color50 = colorPalette[4]; | ||
| const color70 = colorPalette[6]; | ||
| const color80 = colorPalette[7]; | ||
|
|
||
| const mainColor = this.isDark(color) ? color30 : color; | ||
| let mainColor = this.isDark(primaryColor) ? primaryColor : color30; |
There was a problem hiding this comment.
same here - check if isDark() once and apply the changes to mainColor if dark is true (instead of checking isDark() twice in this case).
There was a problem hiding this comment.
I rewrote this condition and ended up with this one:
const mainColor = this.isDark(primaryColor) === !!dark ? color30 : primaryColor
because I want to use color30 only if the color is light in light mode or dark in dark mode, but I'm not sure if it makes the code more readable than that:
let mainColor = isPrimaryColorDark ? primaryColor : color30;
if (dark) {
mainColor = isPrimaryColorDark ? color30 : primaryColor;
}
WDYT?
There was a problem hiding this comment.
Yes, I think this is readable:
const isPrimaryColorDark = this.isDark(primaryColor)
let mainColor = isPrimaryColorDark ? primaryColor : color30;
if (dark) {
mainColor = isPrimaryColorDark ? color30 : primaryColor;
}
|
@lidord-wix |
* fix loadDesignTokens API * review fixes * update generateDesignTokens
Done :) |
Description
Fix Colors.loadDesignTokens API
@Effanuel FYI - we decided to change the API a bit to support adding features in the future easily
Changelog
Fix Colors.loadDesignTokens API