Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added isValid function #633

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caoimghgin
Copy link

@caoimghgin caoimghgin commented Feb 8, 2025

Would be good to have an isValid helper #316 is needed to prevent crashes as users are typing where we want the UI to update upon valid entry dynamically.

Copy link

netlify bot commented Feb 8, 2025

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit fa1962a
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/67a7d49127d6c40008f9d82b
😎 Deploy Preview https://deploy-preview-633--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.

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.

Hi there @caoimghgin, thanks for submitting a PR!

I think a helper for this makes sense, but we absolutely cannot replicate all color parsing logic in it. The helper should most likely just call parse() and catch any thrown errors, i.e. a much, much smaller thing 😁

And since it would be calling the existing parse() machinery, it won't need that many tests either.

In terms of naming, I'd probably call it isColor(). isValid() makes sense in context, but when folks import it and use it throughout their code (which may do other things too), isValid() is less clear (is valid what?). And when used as a static method, it follows the Array.isArray() pattern too.

I'm also not sure what apps/tracer.js is for?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants