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

Fix parsing 7 character hex colors successfully #616

Conversation

kleinfreund
Copy link
Contributor

@kleinfreund kleinfreund commented Dec 18, 2024

Changes

Fix parsing color strings in hexadecimal notation with 7 character successfully. This is caused by the sRGB color space's hex test regexp allowing a match of 3 valid characters followed by a match of 4 valid characters (and vice-versa). The fix is done by separating the regexp for the collapsed and non-collapsed forms.

Add four tests for invalid hex color strings with invalid lengths. Three of them (those for 2, 5, and 9 characters) pass without this fix. Only the one for 7 characters doesn't already.

Notes

  • I can't get the tests to run in my Windows environment using Terminal+PowerShell. I'm getting a Error importing tests from C:\Users\phil\dev\color.js\test\adapt.js: Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:' error for all test files. In my Ubuntu environment, they work however. It looks like there might be a dynamic import somewhere that is fed with a c:\...-style path but I couldn't find it in this repository or htest.

Fix parsing color strings in hexadecimal notation with 7 character successfully. This is caused by the sRGB color space's tex test regexp allowing a match of 3 valid characters followed by a match of 4 valid characters (and vice-versa). The fix is done by separating the regexp for the collpased and non-collapsed forms.

Add four tests for invalid hex color strings with invalid lengths. Three of them (those for 2, 5, and 9 characters) pass without this fix. Only the one for 7 characters doesn't already.
Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 3d4de5a
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/6762beb2af8c7e000848ced1
😎 Deploy Preview https://deploy-preview-616--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.

@kleinfreund kleinfreund marked this pull request as ready for review December 18, 2024 12:24
@LeaVerou
Copy link
Member

I can't get the tests to run in my Windows environment using Terminal+PowerShell.

@DmitrySharabin could you please investigate if this is an hTest bug?
If not, we should document how to run the tests in CONTRIBUTING.md.

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.

The change LGTM, thank you! Someone should run the tests before merging though.

@lloydk
Copy link
Collaborator

lloydk commented Dec 23, 2024

The change LGTM, thank you! Someone should run the tests before merging though.

The tests pass

@lloydk lloydk merged commit 7bc6ff9 into color-js:main Dec 23, 2024
5 checks passed
@lloydk
Copy link
Collaborator

lloydk commented Dec 23, 2024

I can't get the tests to run in my Windows environment using Terminal+PowerShell.

@DmitrySharabin could you please investigate if this is an hTest bug? If not, we should document how to run the tests in CONTRIBUTING.md.

I pushed a commit to upgrade to the latest version of htest. @kleinfreund can you try running the tests again with the latest version of htest. I'm guessing the tests will still fail but it's worth giving it a shot.

@DmitrySharabin
Copy link
Member

I can't get the tests to run in my Windows environment using Terminal+PowerShell.

@DmitrySharabin could you please investigate if this is an hTest bug? If not, we should document how to run the tests in CONTRIBUTING.md.

It turned out that there is a known issue with paths when used in dynamic imports. So, it's something that should be fixed in hTest. I already sent a PR with the fix.

@DmitrySharabin
Copy link
Member

I can't get the tests to run in my Windows environment using Terminal+PowerShell.

It should be fixed now in the latest version of hTest (v0.0.16).

@kleinfreund kleinfreund deleted the fix/parsing-7-character-hex-colors-successfully branch December 26, 2024 12:59
@kleinfreund
Copy link
Contributor Author

It should be fixed now in the latest version of hTest (v0.0.16).

I can confirm that running npm run test on the latest commit in the colorjs.io main branch fixes the issue of not being able to run tests in Windows.

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.

4 participants