-
Notifications
You must be signed in to change notification settings - Fork 82
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
Tree-shakeable, faster, module-based, procedural API #162
Conversation
I still need to add docs but I think I fixed all the regressions and this should be ready for review. |
benchmarks/ai.html
Outdated
in: color_srgb => color_srgb.inGamut(undefined, {epsilon: 0}), | ||
str: color_srgb => color_srgb.toString() | ||
create: (l, c, h) => ({space: oklch, coords: [l, c, h]}), | ||
to_srgb: color => to(color, "srgb"), |
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.
Does srgb
color space register in to()
?
We can do to(color, srgb)
to avoid registration.
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.
Sure. It doesn't yield much of a difference performance-wise though.
import set from "./set.js"; | ||
|
||
export function lighten (color, amount = .25) { | ||
let space = ColorSpace.get("oklch", "lch"); |
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.
I am not very familiar with ColorSpace
but should we import oklch
color space in this file to be sure that color space was loaded?
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.
ColorSpace.get("oklch", "lch")
loads oklch if it's available, or lch if it's not, i.e. loading oklch progressively enhances how lighten()
works.
src/index-fn.js
Outdated
@@ -0,0 +1,14 @@ | |||
export {default as getColor} from "./getColor.js"; |
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.
If we replace export default
to export
in ./*.js
this file will be smaller.
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.
Yes but default exports are nicer for the individual modules.
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.
Just one comment on need for documentation; no changes requested
color1 = to(color1, space); | ||
color2 = to(color2, space); | ||
|
||
// Gamut map to avoid areas of flat 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.
Should document that GM happens before interpolation here, especially as this is a change from CSS Color 4
Changes are in! Time to do a new release on npm and see how it goes. |
Is it correct that the named contrast functions e.g. |
They should be. If not, it's a bug. But I believe they are exported en masse from |
Okay I'll pull the last version again and look at the typings again. |
There's a file |
Just opened #240 to add the missing export |
Will close #159 once merged
The idea is:
Color
objects.color.js
becomes optionalColor
with a simple method call and then they returnColor
objects instead of plain objects (staying compatible with the current API)index.js
imports everything, includingColor
index-fn.js
imports and re-exports all functionsThis means there is both a tree-shakeable, procedural API that is faster and easier to debug, as well as a slower OO API that can be more readable in certain cases.
The new API resulted in a 4x speed increase in this benchmark inspired by @ai's use case.