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

deltas() function, closes #437 #532

Merged
merged 6 commits into from
May 31, 2024
Merged

deltas() function, closes #437 #532

merged 6 commits into from
May 31, 2024

Conversation

LeaVerou
Copy link
Member

No description provided.

@LeaVerou LeaVerou requested review from facelessuser and svgeesus May 28, 2024 01:14
Copy link

netlify bot commented May 28, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit be14c71
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/665a02e833f16d00086d1fa9
😎 Deploy Preview https://deploy-preview-532--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.

@facelessuser
Copy link
Collaborator

Shouldn't this have special handling for hues?

@svgeesus
Copy link
Member

Shouldn't this have special handling for hues?

Yes, as I said earlier

Copy link
Member

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

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

Needs to calculate delta of the hue arc, not the angular delta

@LeaVerou
Copy link
Member Author

LeaVerou commented May 28, 2024

I envisioned this as a low-level utility function, if users want to make this optimization for hue it's trivial to do so on the deltas returned.

That said, it should definitely constrain the ΔH returned in some way (possibly by also preserving the original). We shouldn't have a color with a hue of 350 and a color with a hue of 0 appear to have a ΔΗ of 350. It seems like this could follow shorter hue, possibly even take an argument for it.

LeaVerou added 4 commits May 31, 2024 11:31
- `hue` option to customize how hues are handled
- `shorter` by default
@LeaVerou LeaVerou requested a review from svgeesus May 31, 2024 15:44
@LeaVerou
Copy link
Member Author

Ok, I’ve made several improvements:

  • A hue interpolation setting, with "shorter" being the default.
  • Fixed a bunch of issues around none handling, and added more tests.

@LeaVerou
Copy link
Member Author

We could have another option for scaling the Δh by taking C/L into account, but not sure how that would work in a generic way.

@facelessuser
Copy link
Collaborator

Looks good to me. I would fix lint issues. related to the files in questions.

Copy link
Collaborator

@facelessuser facelessuser left a comment

Choose a reason for hiding this comment

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

LGTM

@LeaVerou LeaVerou merged commit 25f1c0f into main May 31, 2024
4 of 6 checks passed
@LeaVerou LeaVerou deleted the deltas branch May 31, 2024 17:33
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