-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add isRgbColor validator #1141
Add isRgbColor validator #1141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for your contribution!
@ezkemboi - you can have a second pair of eyes on this one, and we should land it. |
I will review it @profnandaa. |
import assertString from './util/assertString'; | ||
|
||
const rgbColor = /^rgb\((([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5]),){2}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\)$/; | ||
const rgbaColor = /^rgba\((([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5]),){3}(0?\.\d|1(\.0)?|0(\.0)?)\)$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on this implementation.
But, have 1 request, rgb, percentage based
.
E.g rgb(0%,0%,0%)
, I think it is a valid rgb value, which is not covered in this PR.
Here is a reference material https://www.december.com/html/spec/colorrgbaper.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I'll try to adjust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ezkemboi I have doubts how to tackle the percentage based values. On one hand - it's valid rgb, on the other - e.g. in React Native it's not supported. Would you agree for some kind of optional flag, like includePercentValues
that would be by default set to true, but will enable to set percentages as invalid value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree.
@@ -2521,6 +2521,30 @@ describe('Validators', () => { | |||
}); | |||
}); | |||
|
|||
it('should validate rgb color strings', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run npm test
. It will generate files such as validator.js
, validator.min.js
and files in lib/isRgbColor.js
.
I think it might be needed. Not sure, but @profnandaa can confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I wasn't sure where these files came from, so didn't commit them
Please resolve the m/c. |
@profnandaa done |
@pawelkleczek -- can you remove unrelated changes from the PR? |
@profnandaa, sorry, it got messy, did the cleanup and force pushed with one commit |
Just fix the merge conflict and we should land this soonest. |
@profnandaa done |
@pawelkleczek -- another merge conflict, please fix and ping me. |
@profnandaa ping :) |
That would be helpful for e.g. React Native projects, where rgb color is passed as a string.