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

Add isRgbColor validator #1141

Merged
merged 3 commits into from
Feb 13, 2020
Merged

Add isRgbColor validator #1141

merged 3 commits into from
Feb 13, 2020

Conversation

pawelkleczek
Copy link
Contributor

That would be helpful for e.g. React Native projects, where rgb color is passed as a string.

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 your contribution!

@profnandaa
Copy link
Member

@ezkemboi - you can have a second pair of eyes on this one, and we should land it.

@ezkemboi
Copy link
Member

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)?)\)$/;
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Member

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', () => {
Copy link
Member

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.

Copy link
Contributor Author

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

@profnandaa
Copy link
Member

Please resolve the m/c.

@profnandaa profnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Oct 25, 2019
@pawelkleczek10c
Copy link
Contributor

@profnandaa done

@profnandaa
Copy link
Member

@pawelkleczek -- can you remove unrelated changes from the PR?

@pawelkleczek
Copy link
Contributor Author

@profnandaa, sorry, it got messy, did the cleanup and force pushed with one commit

@profnandaa
Copy link
Member

Just fix the merge conflict and we should land this soonest.

@profnandaa profnandaa added ready-to-land For PRs that are reviewed and ready to be landed and removed needs-more-review labels Dec 6, 2019
@pawelkleczek
Copy link
Contributor Author

Just fix the merge conflict and we should land this soonest.

@profnandaa done

@profnandaa
Copy link
Member

@pawelkleczek -- another merge conflict, please fix and ping me.

@pawelkleczek10c
Copy link
Contributor

@profnandaa ping :)

@profnandaa profnandaa merged commit 91c2591 into validatorjs:master Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 needs-update For PRs that need to be updated before landing ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants