The scenario is the following. I get the string from an HTML element via outerHTML and I need to convert the 'rgb(r, g, b)' part into the hexadecimal representation. I tried this with regex and it seems to work but I have a feeling there are better ways to deal with this.
One particular thing I don't like is where I directly access color[1]
from my regex match. I'm open to any feedback.
private cleanupRichText(richText: string) {
const rx = /rgb\((.+?)\)/ig
const color = rx.exec(richText)
if (color !== null) {
const hexparts = color[1].split(',').map(str => parseInt(str.trim(), 10).toString(16))
let hexString = '#'
hexparts.forEach(element => {
hexString += element.length === 1 ? '0' + element : element })
text = text.replace(/rgb\(.+?\)/ig, hexString)
}
}
To give an example input:
'rgb(234, 112, 30)'
Expected output:
'#ea701e'
1 Answer 1
While the given function does work for simple cases, it will not perform as expected if there are multiple colors within richText
. The first color will override all other colors.
First, a look at your implementation:
Why is
richText
used in some places and the (global?)text
in others? It looks like these are expected to be equivalent, so you should just userichText
since it is explicitly passed in.Instead of mutating the global
text
, you should return the replaced result. This makes testing the function easier and debugging much less painful.Why is this a class function? It seems to have well defined inputs & outputs that does not depend on the state of any object.
Instead of manually padding numbers with
element.length === 1 ? '0' + element : element
, you should useelement.padStart(2, '0')
if possible. The polyfill isn't that large if it isn't supported everywhere you need it yet (IE), and it is considerably easier to read.parseInt
ignores leading whitespace, so there's no need to dostr.trim()
.String.prototype.replace
accepts a replace function, which will be called with each match. Use this to extract the rgb values instead of doingre.exec
yourself. In the vast majority of cases, you should not needre.exec
.Should text inside the element really be converted? If you are manipulating HTML as text, you probably shouldn't be. If this
rgb()
text is in thestyle
element of the element, you could be usingelement.getAttribute('style')
instead.What should happen if I wrote
rgb(1, 2, 3, 4, 5, 'asdf')
?
Here is how I would write this function, keeping your assumption that all rgb strings are well formed.
function cleanupRichText(richText) {
return richText.replace(/rgb\((.+?)\)/ig, (_, rgb) => {
return '#' + rgb.split(',')
.map(str => parseInt(str, 10).toString(16).padStart(2, '0'))
.join('')
})
}
const tests = [
['rgb(234, 112, 30)', '#ea701e'],
['rgb(0, 0, 0)', '#000000'],
['RGB(255, 255,255)', '#ffffff'],
['rgb(0, 0, 0) rgb(170, 170, 170)', '#000000 #aaaaaa']
]
for (const [text, expected] of tests) {
const result = cleanupRichText(text)
console.log(result === expected ? 'PASS' : 'FAIL', text, '->', result)
}
-
\$\begingroup\$ Thanks a lot for your feedback and suggestions. The reason why I'm doing this is that I have a richtext editor in html and I have to pass the richtext string to my application in order to render a representation of this richtext. Since the richtext parser only accepts hex strings I have to do this conversion. And yes, for the time being I assume that I only get well formed rgb strings. \$\endgroup\$Philipp– Philipp2018年11月19日 09:52:07 +00:00Commented Nov 19, 2018 at 9:52