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

Add color spaces for HSLuv and HPLuv #404

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

lloydk
Copy link
Collaborator

@lloydk lloydk commented Feb 6, 2024

Add color spaces for HSLuv and HPLuv.

The conversion routines where adapted from the typescript source code in the HSLuv repository.

Tests

The HSLuv repository has test data in json format.

I added a script to generate tests suites for hpluv and hsluv. npm run test:generate-hsluv will create test/hpluv.js and test/hsluv.js. The tests take forever to load/run on my ancient Windows PC when run via npm run test. Running the individual test files is less bad. I've added the files to .gitignore so they aren't committed accidently.

image

In Gamut Tests

I added a test suite for inGamut() and the various methods of specifying the gamutSpace for a color space. HSLuv has a gamutSpace of srgb and HPLuv has a gamutSpace of "self".

Closes #339, closes #284

Copy link

netlify bot commented Feb 6, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit c9a27ac
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/65c47cee6674e2000812e216
😎 Deploy Preview https://deploy-preview-404--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

import { fileURLToPath } from "node:url";
import https from "node:https";

const testDataUrl = "https://raw.githubusercontent.com/hsluv/hsluv/ebe5f6fbe7082e04468a325b4c62f783e35987ad/snapshots/snapshot-rev4.json";
Copy link
Member

Choose a reason for hiding this comment

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

Cool idea to import the tests from that repo!
However, if we're gonna pin it to a specific commit, we may as well just copy the file over and import it in the test. There is however value in loading the latest version of the file. Not sure what route you'd rather go with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test data hasn't changed in 8 years so I guess there's no value in pinning to a specific commit. I don't think the file will ever change and if changes are required a snapshot-rev5.json will be created.

My preference right now would be to do the least amount of work. So either keep it as is or change the testDataUrl to not reference a specific commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Walked the dog and changed my mind.

  • I'll create a test/hsluv directory that contains a copy of the json test data and test files for hpluv and hsluv
  • the test files will load the json test data and create the appropriate tests
  • the test:generate-hsluv script will be deleted
  • npm run test:hsluv will run the hsluv tests

@lloydk
Copy link
Collaborator Author

lloydk commented Feb 7, 2024

I've revamped the tests:

  • no generation step
  • all tests and test data are stored in test/hsluv
  • npm run test:hsluv to run the tests

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

I would move test/hsluv/hpluv.js and test/hsluv/hsluv.js to be directly into test (and they can still import the files they need from hsluv/. Other than that, LGTM.

@lloydk
Copy link
Collaborator Author

lloydk commented Feb 8, 2024

I would move test/hsluv/hpluv.js and test/hsluv/hsluv.js to be directly into test (and they can still import the files they need from hsluv/. Other than that, LGTM.

Done

@LeaVerou LeaVerou merged commit fb7e39a into color-js:main Feb 8, 2024
4 checks passed
@LeaVerou
Copy link
Member

LeaVerou commented Feb 8, 2024

Merged, thanks! 🎉

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.

Add HSLuv color space
3 participants