Modernization to the current CSS rgb/rgba color parser#172
Closed
themojache wants to merge 13 commits intojsdom:mainfrom
Closed
Modernization to the current CSS rgb/rgba color parser#172themojache wants to merge 13 commits intojsdom:mainfrom
themojache wants to merge 13 commits intojsdom:mainfrom
Conversation
Made rgb/rgba more spec compliant: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#values https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgba#values https://developer.mozilla.org/en-US/docs/Web/CSS/alpha-value Both rgb and rgba contain the optional parameter of alpha despite naming convention. R, G, B are required; A is optional for both. Test cases: ["rgb(153,255,153,0.5)","rgb(253, 255, 153)","rgb(255 255 255 / 50.75%)","rgba(255, 255, 255 / 50%)","rgb(255, 255, 255, none)","rgba(none, 255, 255 / none)"] Changes (Line references use my changed code): - Reduce processing, one regex for both cases. [Line 32] - Augment to allow flexibility with parts.length being either 3 or 4. [Line 84, 306-323] - Simplify logic into a longer ternary operator rather than 3 cases. [Line 83-86] - Augment split to take several white space, comma, or / delimiters as per spec. [Line 82, 306] - Alpha can now be a %. - Added support for "none" keyword for a rgba value.
- Simplified handling of RGB into a single map operation. - Refactored so rgba prefix would depend on the array length. - Moved rgbdelimiters to a variable near colorRegEx2. Still missing: Relative value syntax (rgb(from <color> R G B[ / A]))
This reverts commit 5904e91.
…s far as I can see implementation wise in browsers
Moved / from rgba to simply the alpha case. More spec compliant for a minor change in the regex. Expanded the colorRegEx1 to capture the hex color string and have case insensitive matching. Updated the hex parsing to use the regex match group. Logic is mostly the same, I added a check to see if full alpha before adding the alpha value to the list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#values
https://developer.mozilla.org/en-US/docs/Web/CSS/alpha-value
Both rgb and rgba contain the optional parameter of alpha despite naming convention. R, G, B are required; A is optional for both.
Test cases: ["rgb(30% 20% 50%)","rgb(153,255,153,0.5)","rgb(253, 255, 153)","rgb(255 255 255 / 50.75%)","rgba(255, 255, 255 / 50%)","rgb( 255, 255, 255, none )","rgba(none, 255, 255 / none)"]
Remains unsupported: Relative value syntax (rgb(from R G B[ / A]))
It seems like Math.round is used where you use Math.floor in browser implementations of handling percent byte rgb values (see MDN's test case of the first test case).
Thanks for the work in maintaining this library. Hope a few touch-ups are appreciated. Reach out/add commits if you notice any impactful bugs. I focused on making the parser more flexible and reducing code (for individual cases). ✌️
Alternative: #171