Skip to content

Conversation

@bijela-gora
Copy link
Contributor

@bijela-gora bijela-gora commented Mar 11, 2023

Link to the culori lib: https://github.com/Evercoder/culori

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
    • the type definitions was tested in oklch-picker project with following steps:
      • manually remove declare module 'culori/fn' section in the oklch-picker/types.d.ts
      • run pnpm add @types/culori@file:../DefinitelyTyped/types/culori/
      • run pnpm run test
  • Add or edit tests to reflect the change.
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • Represents shape of module/library correctly
  • tslint.json should contain { "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.
  • tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

@typescript-bot typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Mar 12, 2023
@typescript-bot
Copy link
Contributor

@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?

@bijela-gora
Copy link
Contributor Author

@peterblazejewicz I added more tests.

@bijela-gora bijela-gora changed the title [Draft] Types for culori Types for culori Mar 12, 2023
// Definitions by: Bijela Gora <https://github.com/bijela-gora>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped

/// <reference path="./fn/index.d.ts" />
Copy link
Member

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

Copy link
Contributor Author

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterblazejewicz

I don't understand why

Screenshot from 2023-03-14 16-45-19

Copy link
Contributor Author

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.

@typescript-bot typescript-bot added the Check Config Changes a module config files label Mar 14, 2023
@typescript-bot
Copy link
Contributor

@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?

@typescript-bot typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR and removed Where is GH Actions? GH Actions didn't give a response to this PR Check Config Changes a module config files labels Mar 14, 2023
Copy link
Member

@peterblazejewicz peterblazejewicz left a 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!

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Mar 14, 2023
@bijela-gora
Copy link
Contributor Author

Ready to merge

@typescript-bot typescript-bot merged commit e26136a into DefinitelyTyped:master Mar 14, 2023
@bijela-gora bijela-gora deleted the types/for-culori branch March 14, 2023 21:56
@Jeremy-Knudsen
Copy link

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?

https://culorijs.org/api/#color-spaces

xyb XYB color space modeXyb

@Jeremy-Knudsen
Copy link

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?

https://culorijs.org/api/#color-spaces

xyb XYB color space modeXyb

@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.

@bijela-gora
Copy link
Contributor Author

bijela-gora commented Jun 1, 2023

@Jeremy-Knudsen hi

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintainer Approved New Definition This PR creates a new definition package. Self Merge This PR can now be self-merged by the PR author or an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants