Skip to content

Support retrieving color JS value #1956

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 10, 2022
Merged

Support retrieving color JS value #1956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 11 additions & 4 deletions src/style/colors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,17 @@ export class Colors {
}
}

/**
* Return the original color string value by its colorKey based on the current scheme(light/dark)
*/
getColorValue(colorKey: string) {
return Scheme.getScheme(true)[colorKey] ?? this[colorKey];
}

getColorName(color: string) {
return ColorName.name(color)[1];
}

getColorTint(color: string, tintKey: string | number) {
if (_.isUndefined(tintKey) || isNaN(tintKey as number) || _.isUndefined(color)) {
// console.error('"Colors.getColorTint" must accept a color and tintKey params');
Expand Down Expand Up @@ -159,10 +170,6 @@ export class Colors {
return this.getTintedColorForDynamicHex(color, tintKey);
}

getColorName(color: string) {
return ColorName.name(color)[1];
}

getTintedColorForDynamicHex(color: string, tintKey: string | number) {
// Handles dynamic colors (non uilib colors)
let tintLevel = Math.floor(Number(tintKey) / 10);
Expand Down
19 changes: 12 additions & 7 deletions src/style/scheme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ export type SchemeType = 'default' | 'light' | 'dark';
export type SchemeChangeListener = (schemeType?: 'light' | 'dark') => void;

class Scheme {
currentScheme: SchemeType = 'default';
schemes: Schemes = {light: {}, dark: {}};
changeListeners: SchemeChangeListener[] = [];
private currentScheme: SchemeType = 'default';
private schemes: Schemes = {light: {}, dark: {}};
private schemesJS: Schemes = {light: {}, dark: {}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how schemesJS is different from schemes... why we need the two?

Copy link
Collaborator Author

@ethanshar ethanshar Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Did you look how do we store each in loadScheme method? we store different values in some use cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically in schemes we keep the native platformColors
The values that are kept there are not regular strings but a weird object that the native side can handle

That's the reason I also want to keep the plain JS strings in schemesJS
It should serve the Colors.getColorValue method when we need to get the original JS string hex value in some cases.

Why do we need the JS string value?
One of the cases, is when we call the Colors.getColorTint in the private repo.
With the recent PlatformColor support, this method throws an error because Colors.getColorTint expect a string but now gets a weird object.
So this whole PR meant to let us get the original string value so we can pass to Colors.getColorTint

I hope this explains better

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I wasn't clear with my question. I understand the meaning behind the pr and why we want to keep the js schemes. What I was missing is where are you loading these schemesJS with the string values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anywhere we call Colors.loadSchemes (It calls internally Scheme.loadSchemes) and it always accept a maps of strings colors.
We internally mapped them once to platform colors or strings

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

private changeListeners: SchemeChangeListener[] = [];
private usePlatformColors = false;

constructor() {
Expand Down Expand Up @@ -63,8 +64,9 @@ class Scheme {
throw new Error(`There is a mismatch in scheme keys: ${missingKeys.join(', ')}`);
}

const platformColorsSchemes: Schemes = cloneDeep(schemes);
merge(this.schemesJS, schemes);

const platformColorsSchemes: Schemes = cloneDeep(schemes);
forEach(schemes, (scheme, schemeKey) => {
forEach(scheme, (colorValue, colorKey) => {
// @ts-expect-error
Expand All @@ -84,15 +86,18 @@ class Scheme {
});
});
});

merge(this.schemes, platformColorsSchemes);
}

/**
* Retrieve scheme by current scheme type
*/
getScheme() {
return this.schemes[this.getSchemeType()];
getScheme(useJS = false) {
if (useJS) {
return this.schemesJS[this.getSchemeType()];
} else {
return this.schemes[this.getSchemeType()];
}
}

/**
Expand Down