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

feat(token lab): editable color tokens #428

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Jan 27, 2025

Changes

video.mp4

How to Review

Dark mode has poor contrast, review could help there.

Copy link

changeset-bot bot commented Jan 27, 2025

⚠️ No Changeset found

Latest commit: 3fbf88e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

if (color && typeof color === 'object') {
let normalizedColor = color;

// DTCG tokens: convert to Culori format
if (color.colorSpace && Array.isArray(color.channels)) {
normalizedColor = tokenToCulori();
normalizedColor = tokenToCulori(color);
Copy link
Contributor Author

@lilnasy lilnasy Jan 27, 2025

Choose a reason for hiding this comment

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

The color token component would not even render without this change.

video.mp4

Comment on lines +77 to 79
function trimTrailingZeros(value: string) {
return value.replace(/\.0+$/, '').replace(/(?<=\.[^\.]+)0+$/, '');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.2300 => 0.23
56.421 => 56.421
123.00 => 123

Comment on lines +42 to +44
{channels.map((v, i) => <output className={c.channel} key={i}>{trimTrailingZeros(String(v).slice(0, 6))}</output>)}
</div>
<output className={c.alpha}>{trimTrailingZeros(String((color.original.alpha * 100)).slice(0, 6))}%</output>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with 5 digits (6 characters) of precision to match the design, but it feels kinda excessive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if using Intl.NumberFormat() would help simplify this. 🤔

It could just as well complicate it too though. Just a thought.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I’ve gone back-and-forth about this. If we’re shooting for 24-bit color, we need 10–12 digits or so:

1 / 2 ** 24 = 0.000000059604

16-bit color is probably a better target because today many devices are in the ~10 bit color range. 16-bit color is fairly safely-represented at ~7 significant digits.

1 / 2 ** 16 = 0.000015

The fewer decimals you have, the less fidelity and colors will “snap” to the wrong color. The designs probably should have more decimals than they do now.

Also +1 I think Intl.NumberFormat() would probably be a better choice that’s more optimized by the JS engine. The RegEx isn’t slow per-se, but on higher refresh rate devices every little slowdown could result in dropping frames while sliding.

Comment on lines +53 to +56
@starting-style {
opacity: 0;
translate: 0 2rem;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

First starting style rule of the codebase 🎉

background: var(--color);
.popover {
display: grid;
background: var(--tz-color-bg-1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

This should be using an elevated token instead, but I couldn't find an analogue. The color picker is also hardcoded to this token anyway. Elevated tokens seems like a broad effort.

Comment on lines -14 to +26
outline: 2px solid var(--tz-color-focus);
outline: 2px solid var(--tz-color-base-lime-800);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This token does not exist. Could there be others leftover from refactors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes we’ve gone through a few refactors and there are some stragglers. Thanks for fixing!

Comment on lines +4 to +7
width: 28rem;
height: 1.5rem;
gap: 1rem;
grid-template-columns: 3fr 20fr 5fr;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most parameters for the layout of the row are centralized here.

@@ -40,6 +40,7 @@
outline: none;
padding: 0;
text-indent: var(--tz-space-200);
field-sizing: content;
Copy link
Contributor Author

@lilnasy lilnasy Jan 27, 2025

Choose a reason for hiding this comment

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

Fixes a layout issue on chromium

Screen.Recording.2025-01-27.093656.mp4

Copy link
Collaborator

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

This is FANTASTIC, thank you! 🎉 This looks and works great; thanks for contributing to the project!

The suggestion on the decimal formatting is not blocking; I left some thoughts but I really just need to benchmark it because there might not be an issue. Performance is really weird in that the real slowdowns are rarely the things you initially think they’d be. At any rate, not blocking at all.

Thanks again!

@drwpow drwpow merged commit bc76b3c into terrazzoapp:main Jan 28, 2025
5 of 6 checks passed
@lilnasy lilnasy deleted the editable-color-tokens branch January 29, 2025 14:25
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.

3 participants