-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
[Performance] Improve tint and shade performance #626
base: main
Are you sure you want to change the base?
[Performance] Improve tint and shade performance #626
Conversation
reduce the amount of if checks for default color
reduce the amount of if checks for default color
Codecov Report
@@ Coverage Diff @@
## main #626 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 89 89
Lines 860 860
Branches 323 323
=========================================
Hits 860 860
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -27,7 +27,7 @@ import mix from './mix' | |||
|
|||
function shade(percentage: number | string, color: string): string { | |||
if (color === 'transparent') return color | |||
return mix(parseFloat(percentage), 'rgb(0, 0, 0)', color) | |||
return mix(parseFloat(percentage), '#000000', color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thadeucity Any reason that we shouldn't send this as a shorthand #000
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The the shorthand one will have two extra Regex validations and two extra if validations (because the first check is for the normal 6 digit hex and the second one is for the 8 digit hex.
@@ -27,7 +27,7 @@ import mix from './mix' | |||
|
|||
function tint(percentage: number | string, color: string): string { | |||
if (color === 'transparent') return color | |||
return mix(parseFloat(percentage), 'rgb(255, 255, 255)', color) | |||
return mix(parseFloat(percentage), '#FFFFFF', color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thadeucity Same as with mix
, should we send the shorthand #fff
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @thadeucity, thank you for taking the time to contribute. A couple small changes, and we should be abe to merge this for inclusion in our next release.
Improve the performance of the
tint
andshade
functions.It is a tiny performance improvement and lib size reduction, but it does not degrade in any shape or form the quality of the code, so I believe it is a welcome change.
The improvement is achieved by exiting the
parseToRgb
function in the firstIF
that checks forHEX Colors
, this way not testing or matching the default color unnecessarily.