Skip to content

[spaces/helmgenlch] Add cssId for color() serialization#731

Open
Grkmyldz148 wants to merge 1 commit into
color-js:mainfrom
Grkmyldz148:fix/helmgenlch-cssid
Open

[spaces/helmgenlch] Add cssId for color() serialization#731
Grkmyldz148 wants to merge 1 commit into
color-js:mainfrom
Grkmyldz148:fix/helmgenlch-cssid

Conversation

@Grkmyldz148
Copy link
Copy Markdown
Contributor

Summary

Adds cssId: "--helmgenlch" to the HelmGenLCh color space registration. Without it, the serializer fell back to space.id ("helmgenlch") and produced an invalid color(helmgenlch …) — CSS Color Module Level 4 requires a dashed-ident for custom color() spaces.

// Before
new Color("helmgenlch", [0.5, 0.1, 30]).toString();
// → "color(helmgenlch 0.5 0.1 30)"   ❌ invalid CSS

// After
// → "color(--helmgenlch 0.5 0.1 30)"  ✅

The chosen identifier (--helmgenlch) mirrors the convention already used by the sibling spaces (--helmgen, --helmlab-metric).

Test plan

  • npm test — all 770 tests pass (4 new, 0 regressions)
  • npx eslint clean on changed files
  • npx prettier --check clean

Added regression coverage in test/serialize.js for the dashed-ident output of helmgen, helmgenlch, and helmlab-metric (with and without alpha) so this can't silently regress again.

Fixes #730

@netlify
Copy link
Copy Markdown

netlify Bot commented May 5, 2026

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit b68bd4d
🔍 Latest deploy log https://app.netlify.com/projects/colorjs/deploys/69fba8779202af000842655e
😎 Deploy Preview https://deploy-preview-731--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

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

Copy link
Copy Markdown
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.

LGTM but for transparency, please don't remove Claude's attribution from commits/PRs, see #723

Without cssId, HelmGenLCh fell back to space.id ("helmgenlch") which
lacks the dashed-ident prefix required by CSS Color Module Level 4
for custom color() spaces. The serializer produced an invalid
"color(helmgenlch ...)" instead of "color(--helmgenlch ...)".

Adds cssId: "--helmgenlch" matching the convention used by sibling
spaces (--helmgen, --helmlab-metric).

Also adds regression coverage in test/serialize.js for the dashed-ident
output across all three custom Helm color() spaces.

Fixes color-js#730

Co-Authored-By: Claude <noreply@anthropic.com>
@Grkmyldz148 Grkmyldz148 force-pushed the fix/helmgenlch-cssid branch from 3e499ed to b68bd4d Compare May 6, 2026 20:45
@Grkmyldz148
Copy link
Copy Markdown
Contributor Author

Apologies @LeaVerou — completely my oversight, thanks for the gentle reminder.

Force-pushed an amend that adds the Co-Authored-By: Claude trailer to the commit (per #723). I've also written this down as a personal rule so the attribution stays on every future commit and PR body in this repo. 🙏

@LeaVerou
Copy link
Copy Markdown
Member

LeaVerou commented May 7, 2026

Approved, thanks! If you're feeling particularly generous with your time, you could take a stab at codifying #723 in CONTRIBUTING.md 😁

@Grkmyldz148
Copy link
Copy Markdown
Contributor Author

Thanks! I’ll definitely do that 😊

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.

Bug: HelmGenLCh not serialized to color() with a dashed-ident

2 participants