-
Notifications
You must be signed in to change notification settings - Fork 30.5k
Types for culori #64686
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
Types for culori #64686
Conversation
…@types/culori:file:../path/to/DefinitelyTyped/types/culori`
…nature with an optional parameter." error
|
@peterblazejewicz Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
|
@peterblazejewicz I added more tests. |
types/culori/index.d.ts
Outdated
| // Definitions by: Bijela Gora <https://github.com/bijela-gora> | ||
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
|
|
||
| /// <reference path="./fn/index.d.ts" /> |
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.
so this line is here just to make sure culori/fn symbols are exported?
If so, you can consider removing this one, adding 'OTHER_FILES.TXT' file and referencing fn/index.d.ts in it:
https://github.com/DefinitelyTyped/DefinitelyTyped#other_filestxt
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.
Yeah it was for that.
I added a OTHER_FILES.txt and the npm run test-all return the following error 'fnindex.d.ts listed in OTHER_FILES.txt is already reachable from tsconfig.json'.
So I removed imports of the 'culori/fn` in test files.
Thanks for noticing this!
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.
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 just removed /// <reference path="./fn/index.d.ts" /> line.
|
@peterblazejewicz Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
peterblazejewicz
left a comment
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.
LGTM!
@bijela-gora thanks!
|
Ready to merge |
|
Great to see this update, but one thing I see missing is support for the added color space XYB which is a perceptual color space used in JPEG XL. I was hoping this update would have included it. Any chance it could be added?
|
@bijela-gora Is there any chance you can add the XYB color space that is new to culori to the type definitions? :) @mentioning you in case you didn't see my message. |
|
I have no plans to support this types definitions. Adding types for the culori lib was résumé-driven project. I am open to transferring ownership. Also I opened for reviewing PRs. |

Link to the culori lib: https://github.com/Evercoder/culori
Please fill in this template.
declare module 'culori/fn'section in theoklch-picker/types.d.tspnpm add @types/culori@file:../DefinitelyTyped/types/culori/pnpm run testnpm test <package to test>.Select one of these and delete the others:
If adding a new definition:
.d.tsfiles generated via--declarationdts-gen --dt, not by basing it on an existing project.tslint.jsonshould contain{ "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.tsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.