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

Added a quick fix for tetrad array element selection bug. #105

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

tabuckner
Copy link
Contributor

@tabuckner tabuckner commented Jan 28, 2019

@mbitson @fireflight1 @simon04 Discovered this while making a SASS adaptation of your approach. Thank you so much for making this open source! It's still not perfect, and might not look good for all colors, but I feel it's a closer approximation.

tinycolor.tetrad() returns an array with a length of 4. Targeting index [4] returns undefined. tinycolor.mix() does not complain about an undefined value for argument 2, but we're not correctly handling the mix approach.

Using @angular/material as a baseline, I tweaked the values to somewhat more closely match the mat-indigo palette.

// @angular/material mat-indigo palette
mat-indigo = {
  50: #e8eaf6,
  100: #c5cae9,
  200: #9fa8da,
  300: #7986cb,
  400: #5c6bc0,
  500: #3f51b5,
  600: #3949ab,
  700: #303f9f,
  800: #283593,
  900: #1a237e,
  A100: #8c9eff,
  A200: #536dfe,
  A400: #3d5afe,
  A700: #304ffe
};
// new output with same base color
testMap = {
  50: #e8eaf6,
  100: #c5cbe9,
  200: #9fa8da,
  300: #7985cb,
  400: #5c6bc0,
  500: #3f51b5,
  600: #394aae,
  700: #3140a5,
  800: #29379d,
  900: #1b278d,
  A100: #88a6ff,
  A200: #4b79ff,
  A400: #3166ff,
  A700: #225aff
};

Mat-Indigo
image

New values
image

Copy link
Owner

@mbitson mbitson left a comment

Choose a reason for hiding this comment

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

Hey @tabuckner !

Thank you so much for your interest in the project and your commit here! You're adjusting one of the particular color algorithms, where instead you should be making a third algorithm option for the settings. At that point we'd need to adjust the if{}else{} statement to be more robust and call any one of the three possible algorithms for color generation.

If we did that I'd go ahead and merge this in. I don't have a lot of time lately, but I may come back and help adjust.

Thanks!

@tabuckner
Copy link
Contributor Author

@mbitson sure, i can do that! One sec

@tabuckner
Copy link
Contributor Author

tabuckner commented Jan 28, 2019

@mbitson made a few changes, and I think this oughta do the trick:

Left: 'Buckner', Right: 'Constantin'
image

Copy link
Owner

@mbitson mbitson left a comment

Choose a reason for hiding this comment

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

Hey @tabuckner !

Impressive work, thank you sir!

I'll get this deployed to my copy of the tool soon.

@tabuckner
Copy link
Contributor Author

@mbitson No worries :], I'm glad I was able to contribute! You helped me out of a bind by making this OS!

@mbitson mbitson merged commit 71540f6 into mbitson:develop Apr 29, 2020
@mbitson
Copy link
Owner

mbitson commented Apr 29, 2020

Thank you again for this fix, @tabuckner ! Getting this repo cleaned up and this is helpful!

@tabuckner
Copy link
Contributor Author

Of course!! Thank you for what you've done!

Let me know if you want any help I'd love to give back 😄

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