Skip to content

Infra/ revese color palettes in dark mode #1893

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

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

lidord-wix
Copy link
Contributor

Description

Reverse color palettes in dark mode
WOAUILIB-2585

Changelog

Reverse color palettes in dark mode

@@ -11,6 +11,7 @@ import Scheme, {Schemes, SchemeType} from './scheme';

export class Colors {
[key: string]: any;
shouldSupportDarkMode = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check if you can mark this class member as private as private houldSupportDarkMode = false;
I think it will avoid having this member exposed to the user

@@ -132,7 +134,8 @@ export class Colors {
const colorKey = _.findKey(this, (_value, key) => this[key] === color);

if (colorKey) {
const requiredColorKey = `${colorKey.slice(0, -2)}${tintKey}`;
const key = this.shouldSupportDarkMode && Scheme.getSchemeType() === 'dark' ? 90 - Number(tintKey) : tintKey;
Copy link
Collaborator

@ethanshar ethanshar Mar 15, 2022

Choose a reason for hiding this comment

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

I'm not so sure about the 90 - Number(tintKey) part?
Its purpose is to reverse the key to the dark mode key?
Since now we added keys like 1 and 5 it's possible we'll get a dark-mode key of 89 or 85

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@lidord-wix lidord-wix requested a review from ethanshar March 15, 2022 15:33
@@ -143,6 +147,24 @@ export class Colors {
return this.getTintedColorForDynamicHex(color, tintKey);
}

getInvertedTintKey(tintKey: string | number) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can I offer a code improvement (:

You can create an array of our keys ([1, 5, 10, 20, 30, ..., 80])
Then use array.indexOf to detect the given key index, revert the index (as you did before) and return the inverted key..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@lidord-wix lidord-wix requested a review from ethanshar March 17, 2022 09:25
@ethanshar ethanshar merged commit 6183f55 into master Mar 21, 2022
@lidord-wix lidord-wix deleted the infra/reverse_colors_in_darkMode branch April 18, 2022 12:00
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.

2 participants