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 #2804: Use chroma-js for all color parsing and conversion #8000

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

f3rnando0
Copy link

Summary

Refactered color parsing and conversion functions to use chroma-js methods as described in the issue #2804.

General checklist

  • Methods changed
    • hexToHsv
    • hexToRgb
    • hsvToHex
    • hsvToRgb
    • rgbToHex
    • isValidHex - Didn't change this function because chroma-js.valid method allows hex strings that don't start with #, which implies with the tests, so I just added a comment about it and remained the regex test.

@f3rnando0 f3rnando0 requested a review from a team as a code owner September 4, 2024 13:00
Copy link

cla-checker-service bot commented Sep 4, 2024

💚 CLA has been signed

Copy link

github-actions bot commented Sep 4, 2024

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

Copy link

github-actions bot commented Sep 4, 2024

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

@tkajtoch
Copy link
Member

tkajtoch commented Sep 9, 2024

Thanks for your contribution, @f3rnando0! Before we review it, we need to consider potential performance implications this may cause. Chroma.js is pretty slow when converting certain color formats, resulting in highly increased emotion style function evaluations.

I've experienced ~3ms conversion times running on M2 Pro that I consider the top end of our users' CPUs. Since the styles need to be computed before the initial component render, this delay blocks components from being rendered "immediately" (let's forget about the react scheduler for the sake of this conversation), and due to the synchronous nature of how Emotion styles are being loaded, child components' functions can only be executed once Chroma finishes its calculations risking creating a cascading effect when used in multiple nested components.

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

Successfully merging this pull request may close these issues.

2 participants