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.

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

Jump to bottom

Colors - adjust palette to steps of 9 when addDarkestTint is true #2855

Merged
merged 12 commits into from Dec 20, 2023
Merged

Colors - adjust palette to steps of 9 when addDarkestTint is true #2855

merged 12 commits into from Dec 20, 2023

Conversation

Inbal-Tish
Copy link
Collaborator

Description

Colors - adjust palette to steps of 9 when addDarkestTint is true

Changelog

Colors - adjust palette to steps of 9 when addDarkestTint is true

Additional info

WOAUILIB-3830

@Inbal-Tish Inbal-Tish added the Important for Next Release PR that must be included in the release version label Dec 17, 2023
M-i-k-e-l
M-i-k-e-l reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

PLEASE try to refactor in separate commits 🙏
I think it would have been much easier to look at.

src/style/__tests__/colors.spec.js Outdated
const palette = uut.generateColorPalette('#FFE5FF');
expect(palette).toEqual(['#661A66', '#8F248F', '#B82EB7', '#D148D1', '#DB71DB', '#E699E6', '#F0C2F0', '#FFE5FF']);
it('should generateColorPalette with adjustLightness option true (default)', () => {
const palette = uut.generateColorPalette(baseColor, {adjustLightness: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little odd IMO, you send the default value and test it, shouldn't the test above be enough for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I wanted to explicitlly test it but it is redundent

src/style/__tests__/colors.spec.js Outdated
const tints = ['#193852', '#255379', '#316EA1', '#3F88C5', '#66A0D1', '#8DB9DD', '#B5D1E9', '#DCE9F4'];
const baseColorLight = '#DCE9F4';
const tintsLight = ['#1A3851', '#265278', '#326D9F', '#4187C3', '#68A0CF', '#8EB8DC', '#B5D1E8', '#DCE9F4'];
const tintsAdjustedLightness = ['#193852', '#255379', '#316EA1', '#3F88C5', '#66A0D1', '#8DB9DD', '#B5D1E9', '#DCE9F4'];
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there is duplication of consts, why do we need them?
tintsAdjustedLightness = tints
tintsAdjustedSaturation = tintsLight

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I see... That's because I tested default options... I removed the tests

src/style/colors.ts Outdated
@@ -323,7 +337,31 @@ function colorStringValue(color: string | object) {
return color?.toString();
}

function adjustSaturation(colors: string[], color: string) {
function adjustAllSaturations(colors: string[], color: string, levels: number[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is color: string actually selectedColor`currentColor`?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes... changed to 'baseColor' to be extra clear

src/style/colors.ts Outdated
const hsl = Color(c).hsl();
const saturation = hsl.color[1];
const level = levels[index];
if (level) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will ignore 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fine... Changed to be more explicit?

src/style/colors.ts Outdated
if (level) {
const saturationLevel = saturation + level;
const clampedLevel = _.clamp(saturationLevel, 0, 100);
const adjusted = addSaturation(c, clampedLevel);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's out of scope but I think addSaturation is actually setSaturation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree

demo/src/screens/foundationScreens/ColorsScreen.js Outdated
render() {
return (
<>
{this.renderSearch()}
<ScrollView ref={this.scrollViewRef}>
{/* {this.renderColorPaletteExample()} */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why it's hidden, wouldn't UX will want to test it?
BTW, I did not test it.
We can go over it together if you like

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to the bottom of the screen

@M-i-k-e-l M-i-k-e-l merged commit 4f78607 into master Dec 20, 2023
1 check passed
Inbal-Tish added a commit that referenced this pull request Dec 20, 2023
@Inbal-Tish
Revert "Colors - adjust palette to steps of 9 when addDarkestTint is …
8381b50 
…true (#2855)"

This reverts commit 4f78607.
@Inbal-Tish Inbal-Tish mentioned this pull request Dec 20, 2023
Closed
@Inbal-Tish Inbal-Tish deleted the infra/Colors_palette_generator branch January 14, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M-i-k-e-l M-i-k-e-l M-i-k-e-l approved these changes

Assignees

@M-i-k-e-l M-i-k-e-l

Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Inbal-Tish @M-i-k-e-l