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

Added feature isHSL #1159

Merged
merged 25 commits into from
Feb 23, 2020
Merged

Added feature isHSL #1159

merged 25 commits into from
Feb 23, 2020

Conversation

ayala-io
Copy link
Contributor

As suggested in #1156, I made an isHSL validator. It accepts padded zeroes and white spaces. I basically tried playing around with https://www.w3schools.com/colors/tryit.asp?filename=trycolors_hsl and checked what worked and what didn't work.

Summary of changes:

  • src/lib/isHSL.js: Source of validator
  • src/index.js: Modified to recognize newly created isHSL
  • test/validators.js: Added test cases for isHSL
  • README.md: Added short description of isHSL

@ezkemboi
Copy link
Member

@kurisu-gh, I will review this PR.
Also, try run tests and push the changes.

@ghost
Copy link

ghost commented Dec 27, 2019

Should values greater than 360 and lesser than 0 be invalid as the first parameter (hue)? Since it's a circle, you just have to account turns in order to get the right value. If you try this out in chrome devtools you'll see that the browser handles turns in the circle.

@ayala-io
Copy link
Contributor Author

ayala-io commented Dec 27, 2019

Should values greater than 360 and lesser than 0 be invalid as the first parameter (hue)? Since it's a circle, you just have to account turns in order to get the right value. If you try this out in chrome devtools you'll see that the browser handles turns in the circle.

You're right. I confirmed this myself in https://www.w3schools.com/colors/tryit.asp?filename=trycolors_hsl

I just pushed some modifications to this PR that allows hue values larger than 360 and less than 0. +/- signs are also validated. I added some more test cases considering that modulo 360 is possible.

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

@profnandaa
Copy link
Member

@ezkemboi -- please have a look at this again.

@profnandaa profnandaa added needs-more-review ready-to-land For PRs that are reviewed and ready to be landed labels Feb 6, 2020
@ezkemboi
Copy link
Member

ezkemboi commented Feb 6, 2020

Alright, checking @profnandaa

Copy link
Member

@ezkemboi ezkemboi left a comment

Choose a reason for hiding this comment

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

I left my feedback here @kurisu-gh, @GuiSchafer, and @profnandaa.

test/validators.js Show resolved Hide resolved
@ezkemboi ezkemboi mentioned this pull request Feb 15, 2020
@profnandaa profnandaa added 🧹 needs-update For PRs that need to be updated before landing and removed ready-to-land For PRs that are reviewed and ready to be landed labels Feb 22, 2020
@profnandaa
Copy link
Member

@kurisu-gh -- ping!

@ayala-io
Copy link
Contributor Author

ayala-io commented Feb 22, 2020

Sorry for the delay! Was bogged down with work.

I've pushed some updates and synced with upstream. This PR now handles the test cases that @ezkemboi brought up. But upon further reviewing of the MDM documentation, validating HSL is a bit more involved than I thought. To summarize:

  • HSL actually supports the alpha parameter (optionally) previously mentioned in Added feature isHSL #1159 (comment) and I see there is now a feature request Add isHSLA() #1245. As of CSS Colors Level 4, hsla() is merely an alias to hsl(). If you use hsla() without the alpha parameter, it would still work fine due to hsl() alias. In other words, isHSL() needs to validate the optional 4th parameter. In this PR, I added the 4th parameter check. I think we no longer need an isHSLA() validator because hsla() is really hsl() or we can have isHSLA() as alias to isHSL().
  • There is a comma-separated and a space-separated way to express hsl(). Comma-separated is straight forward, but space-separated adds the 4th parameter (alpha) with a / delimiter. Space-separated can actually have no spaces at all like: hsl(200grad+.1%62%/1) . It gets complicated when taking into account that the optional +/- is like a space separator. My PR doesn't fully capture all the cases.
  • Percentage values and alpha values [0,1] (or percentage) can be actually be set to values beyond their normal bounds. They will just saturate to 0% (0.0) or 100% (1.0) in practice. This PR now allows values beyond the normal bounds.
  • My regex patterns are a bit brute force right now. It would be nicer to break them down into reusable pieces. Many parts of my patterns are repeated. For example, checking for a CSS <number> which has an optional +/-, an integer followed by an optional scientific notation portion; or it can be a (+/- optional) float with optional leading 0's before the decimal, followed by an optional scientific notion portion. I think there are definitely smarter, more modular ways to express the patterns here.
  • More test cases should be considered.

So in short, the PR handles the additional checks requested by @ezkemboi as well as the optional alpha parameter. More work is needed to handle space-separated validation. I can continue development of this feature but it will be slow. If anyone wants to take over development of this, feel free. Also open to suggestions on simplifying my regex patterns.

@profnandaa
Copy link
Member

@ezkemboi -- ping ^

@profnandaa profnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Feb 23, 2020
Copy link
Member

@ezkemboi ezkemboi 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 good @profnandaa. @kurisu-gh has done even extra research and work which is amazing.
Good work @kurisu-gh and thanks once again.

@ayala-io
Copy link
Contributor Author

Thank you for reviewing @ezkemboi and @profnandaa. Let me just update the readme in a little bit to note that an optional alpha parameter is also supported.

@ayala-io
Copy link
Contributor Author

@ezkemboi @profnandaa, updated the README.md appropriately and synced up the PR as well. Thank you for the efforts!

README.md Outdated Show resolved Hide resolved
@profnandaa profnandaa merged commit b1a3b4f into validatorjs:master Feb 23, 2020
@ayala-io ayala-io deleted the add-feature-isHSL branch February 23, 2020 14:55
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.

3 participants