feat(isRgbColor): add `allowSpaces` option to allow/disallow spaces between color values #2029

a-h-i · 2022-08-15T15:07:02Z

isRgbColor now behaves in a similar manner to HSL, and ignores spaces. fixes #2028

Stripped spaces in isRgbColor function

Checklist

PR contains only changes related; no stray files, etc.
README updated (where applicable)
Tests written (where applicable)

codecov · 2022-08-15T15:09:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83d6ffd) to head (97c1f70).
Report is 2 commits behind head on master.

❗ Current head 97c1f70 differs from pull request most recent head bfca688. Consider uploading reports for the commit bfca688 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            master     #2029    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          108       104     -4     
  Lines         2482      2215   -267     
  Branches       627       481   -146     
==========================================
- Hits          2482      2215   -267

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

braaar · 2022-08-16T05:33:30Z

test/validators.js

+        'rgba(255, 255, 255, 0.5)',
      ],


I think it's worthwhile to add a test case like this for clarity's sake:

Suggested change

'rgba(255, 255, 255, 0.5)',

],

'rgba(255, 255, 255, 0.5)',

'r g b( 0, 251, 222 )',

],

changed so that it does not ignore spaces between rgb at start, and added the test cases

WikiRik · 2022-08-16T13:24:08Z

src/lib/isRgbColor.js

@@ -4,16 +4,19 @@ const rgbColor = /^rgb\((([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5]),){2}
 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)?)\)$/;
 const rgbColorPercent = /^rgb\((([0-9]%|[1-9][0-9]%|100%),){2}([0-9]%|[1-9][0-9]%|100%)\)/;
 const rgbaColorPercent = /^rgba\((([0-9]%|[1-9][0-9]%|100%),){3}(0?\.\d|1(\.0)?|0(\.0)?)\)/;
+const startsWithRgb = /^rgba?/;


Why did you add this? Isn't it already checked in the first section of the other regexes?

Ah, I see. It's because the newly added test is invalid. Why is that?

so that we can check that it does not haves spaces between r g b and a only for the rest of the string

I figured 'r g b( 0, 251, 222 )' failing validation but 'rgb( 0, 251, 222 )' passing is more inline with what an rgb color string is

braaar

LGTM!

test/validators.js

braaar · 2022-08-17T07:31:05Z

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

test/validators.js

a-h-i · 2022-08-17T16:51:10Z

Could it be useful to have a strict mode for this validator? I can picture a case where someone needs an RGB string formatted without spaces (or up to 1 space after commas between the numbers, perhaps). For those people this change would not only be a breaking change, but it would make the validator useless.

Strictness has come up a few times before, and there is an ongoing process with the ISO8601 validator which stems from people expecting different levels of strictness (the ISO8601 standard includes a lot of different formats, but people often want only one specific timestamp format).

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

braaar · 2022-08-18T08:39:46Z

it's possible to add a boolean parameter and switch between old and new behaviour. My intent was just to resolve the issue faced in #2028

I think that is the safest approach. If we want to avoid breaking changes, the function should take an options object that defaults to { strict = true } which does not allow whitespace, and you have to set it to false to allow whitespaces.

Personally I would lean towards having it default to false and just do a major version bump. It just makes more sense that it is lax by default and you have to actively make it stricter. In that case you should write a clear descriptor of what the breaking change entails. I suggest changing the title of the PR to fix!: allow whitespace between color values in isRgbColor, add strict mode.

WikiRik · 2022-08-18T15:16:59Z

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

braaar · 2022-08-19T10:55:59Z

I agree that we want the default to be false, but I think we can go forward with this PR with strict = true and add a TODO that for the next major release we want to change that behaviour

Perhaps it is more elegant to make a PR for that right away and state in the title that it should be merged for the next major version. It's easy to forget TODOs deep in the source code.

WikiRik · 2022-08-19T11:08:14Z

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

a-h-i · 2022-08-19T12:25:37Z

Making a second PR right after this is merged to keep for the next major release would indeed be better. Good point!

So is this PR good to go?

braaar · 2022-08-19T12:38:21Z

src/lib/isRgbColor.js


-export default function isRgbColor(str, includePercentValues = true) {
+export default function isRgbColor(str, includePercentValues = true, strict = true) {


I would use an options object here instead of individual arguments.

I think something should be done so that the function will be backwards compatible. There are examples of this somewhere in this repo, but I don't remember exactly where I've seen it

We should indeed make includePercentValues backwards compatible if we move to an options object (which has my preference as well, see #1874 ). An example for this is;

validator.js/src/lib/isLength.js

Lines 8 to 14 in 86a07ba

if (typeof (options) === 'object') {

min = options.min || 0;

max = options.max;

} else { // backwards compatibility: isLength(str, min [, max])

min = arguments[1] || 0;

max = arguments[2];

}

is something like 301e707 what you had in mind? 🙂

We don't want to present both ways of providing options in the readme. Just describe the object parameter. The backwards compatibility is just there to avoid breaking changes for existing codebases. I don't think we should actively promote the old (inferior) way.

@braaar updated readme

WikiRik · 2022-08-21T20:12:41Z

src/lib/isRgbColor.js

+      options.includePercentValues : true;
+  } else {
+    // backward compaitable behaviour
+    // Defaults


I think we can make the object the default behaviour, as long as the backwards compatible behaviour keeps working this would not be a breaking change. But I'm interested what @braaar and @rubiin have to say about this

I agree, we should have the object as the default behaviour. That makes the most sense to me.

@a-h-i I still think it's worthwhile to flip the logic here and put the object in the else clause

@braaar changed in 9d0d3b3

braaar

I think this is a clearer explanation

README.md

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

This reverts commit 4c31a86.

This reverts commit 3f05aa5.

This reverts commit 09d9e95.

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

a-h-i · 2024-05-14T21:43:51Z

@profnandaa rebased and resolved merge conflicts

profnandaa

LGTM, thanks for your contrib! 🎉

a-h-i · 2024-05-20T10:08:03Z

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

WikiRik · 2024-05-21T20:57:00Z

@WikiRik hey, sorry for the ping, I was wondering does it require another review? A bit unclear on the process from here

@profnandaa will merge this one soon and then it should end up in the next release

WikiRik previously approved these changes Aug 15, 2022

View reviewed changes

braaar suggested changes Aug 16, 2022

View reviewed changes

a-h-i dismissed WikiRik’s stale review via 69183fe August 16, 2022 12:21

a-h-i requested review from braaar and WikiRik and removed request for braaar and WikiRik August 16, 2022 12:24

WikiRik reviewed Aug 16, 2022

View reviewed changes

braaar previously approved these changes Aug 17, 2022

View reviewed changes

test/validators.js Outdated Show resolved Hide resolved

a-h-i dismissed braaar’s stale review via c79bda6 August 17, 2022 11:18

braaar reviewed Aug 17, 2022

View reviewed changes

test/validators.js Outdated Show resolved Hide resolved

a-h-i requested a review from WikiRik August 18, 2022 23:09

braaar reviewed Aug 19, 2022

View reviewed changes

a-h-i requested review from braaar and WikiRik and removed request for WikiRik and braaar August 21, 2022 05:56

WikiRik reviewed Aug 21, 2022

View reviewed changes

braaar suggested changes Aug 22, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

a-h-i and others added 20 commits May 14, 2024 22:33

isRgbColor no longer ignores spacing between rgba? at start

b3beff3

updates isRGB test cases

bae863f

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

Fixes typo

cbe2bc6

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

adds strict mode to preserve old behaviour

bc534c1

updates README.md with isRgbColor strict param

4eeb798

implements optional options param for isRgbColor

414f743

updates readme

0cc22f8

Update README.md

6a40c9e

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

Update README.md

9343713

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

flips order of object arguments check

4673f58

adds more isRgbColor test examples

5325127

removes strict support for arguments array

1b982fe

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

sets options default in function signature

dd2b080

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

options object defaults now in signature

e10d3a1

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

Revert "options object defaults now in signature"

15f80b8

This reverts commit 4c31a86.

Revert "sets options default in function signature"

718872e

This reverts commit 3f05aa5.

Revert "removes strict support for arguments array"

b27bff7

This reverts commit 09d9e95.

changes strict to be options object only param

7e5225c

Update src/lib/isRgbColor.js

dbd5260

Co-authored-by: Brage Sekse Aarset <brage.aarset@gmail.com>

changes strict to allowSpaces

bfca688

a-h-i force-pushed the master branch from 97c1f70 to bfca688 Compare May 14, 2024 21:42

profnandaa added the 🎉 first-pr label May 15, 2024

profnandaa approved these changes May 15, 2024

View reviewed changes

profnandaa removed the mc-to-land Just merge-conflict standing between the PR and landing. label May 15, 2024

rubiin added enhancement 🎉 first-pr and removed 🎉 first-pr labels May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(isRgbColor): add `allowSpaces` option to allow/disallow spaces between color values #2029

feat(isRgbColor): add `allowSpaces` option to allow/disallow spaces between color values #2029

a-h-i commented Aug 15, 2022 •

edited

codecov bot commented Aug 15, 2022 •

edited

braaar Aug 16, 2022 •

edited

a-h-i Aug 16, 2022

WikiRik Aug 16, 2022

WikiRik Aug 16, 2022

a-h-i Aug 16, 2022

a-h-i Aug 16, 2022

braaar left a comment

braaar commented Aug 17, 2022 •

edited

a-h-i commented Aug 17, 2022

braaar commented Aug 18, 2022

WikiRik commented Aug 18, 2022

braaar commented Aug 19, 2022

WikiRik commented Aug 19, 2022

a-h-i commented Aug 19, 2022

braaar Aug 19, 2022

WikiRik Aug 19, 2022

a-h-i Aug 19, 2022 •

edited

braaar Aug 22, 2022

a-h-i Aug 22, 2022

WikiRik Aug 21, 2022

braaar Aug 22, 2022

braaar Aug 25, 2022

a-h-i Aug 26, 2022

braaar left a comment •

edited

a-h-i commented May 14, 2024

profnandaa left a comment

a-h-i commented May 20, 2024

WikiRik commented May 21, 2024


		export default function isRgbColor(str, includePercentValues = true) {
		export default function isRgbColor(str, includePercentValues = true, strict = true) {

	if (typeof (options) === 'object') {
	min = options.min \|\| 0;
	max = options.max;
	} else { // backwards compatibility: isLength(str, min [, max])
	min = arguments[1] \|\| 0;
	max = arguments[2];
	}

feat(isRgbColor): add allowSpaces option to allow/disallow spaces between color values #2029

Are you sure you want to change the base?

feat(isRgbColor): add allowSpaces option to allow/disallow spaces between color values #2029

Conversation

a-h-i commented Aug 15, 2022 • edited

Checklist

codecov bot commented Aug 15, 2022 • edited

Codecov Report

braaar Aug 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braaar left a comment

Choose a reason for hiding this comment

braaar commented Aug 17, 2022 • edited

a-h-i commented Aug 17, 2022

braaar commented Aug 18, 2022

WikiRik commented Aug 18, 2022

braaar commented Aug 19, 2022

WikiRik commented Aug 19, 2022

a-h-i commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-h-i Aug 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braaar left a comment • edited

Choose a reason for hiding this comment

a-h-i commented May 14, 2024

profnandaa left a comment

Choose a reason for hiding this comment

a-h-i commented May 20, 2024

WikiRik commented May 21, 2024

feat(isRgbColor): add `allowSpaces` option to allow/disallow spaces between color values #2029

feat(isRgbColor): add `allowSpaces` option to allow/disallow spaces between color values #2029

a-h-i commented Aug 15, 2022 •

edited

codecov bot commented Aug 15, 2022 •

edited

braaar Aug 16, 2022 •

edited

braaar commented Aug 17, 2022 •

edited

a-h-i Aug 19, 2022 •

edited

braaar left a comment •

edited