automatically show light or dark logos when using simple-icons #2833
Conversation
lib/logos.js
Outdated
| const svgColor = toSvgColor(color) | ||
| const svgColor = | ||
| toSvgColor(color) || | ||
| toSvgColor(getSimpleIconColor({ icon: simpleIcons[key], style })) | ||
| if (svgColor) { | ||
| return svg2base64(svg.replace('<svg', `<svg fill="${svgColor}"`)) | ||
| } else { |
There was a problem hiding this comment.
The way this is written it looks like we will never end up here. i.e. we will always recompute the svg + the base64 transform.
Should we precompute the whitesmoke and #333 versions when the logos are loaded, instead?
There was a problem hiding this comment.
The way this is written it looks like we will never end up here. i.e. we will always recompute the svg + the base64 transform.
Yeah, think this may be the case.
Should we precompute the
whitesmokeand#333versions when the logos are loaded, instead?
Good idea, it should save a quite a bit of resources in the long run.
and then we would just need to update the replace to something like:
svg.replace(/fill=".+?"/, `fill="${svgColor}"`);Although it could probably be done in a separate PR later on if its too much hassle.
lib/logos.js
Outdated
| } | ||
|
|
||
| function hexToRgb(hex) { | ||
| const result = /^#?([a-f\d]{2})([a-f\d]{2})([a-f\d]{2})$/i.exec(hex) |
There was a problem hiding this comment.
Could you move the regex declaration outside the function?
There was a problem hiding this comment.
I don't see this regex being used outside of this function.
Would it be better to keep it in here until its needed elsewhere?
There was a problem hiding this comment.
I think moving it outside is a performance optimisation. It gets declared once when we start the server process instead of redefining it on every function call
| given({ icon: { hex: '004880' }, style: 'social' }).expect('004880') // dark logo, light background | ||
| given({ icon: { hex: '00AFF0' }, style: undefined }).expect('00AFF0') // medium logo, dark background | ||
| given({ icon: { hex: '00AFF0' }, style: 'social' }).expect('00AFF0') // medium logo, light background | ||
| }) |
There was a problem hiding this comment.
It would be good to have a couple higher-level tests of this functionality. Perhaps you could call add a third test for prepareNamedLogo() with a result you've checked, or else a regular unit test that turns the base64 back into svg and checks that it includes whitesmoke.
This is "unconversion" code I wrote for #2473:
const logoSvg = Buffer.from(
getShieldsIcon({ name: 'github' }).replace('data:image/svg+xml;base64,', ''),
'base64'
).toString('ascii')
|
👍 Nice work, This looks good to me! Just to clarify that if light logo: will keep default color |
Hmm. Good point. The reason that's happening is because if the style is anything other than "social", we're assuming the background is the default and applying the "dark background" logic. Do you think we should explicitly pass through |
|
It should be fine the way you have done it, I think most people would probably find it more unexpected if the logo color suddenly changed when they changed Edit: Although, It would make it harder for people to find the brand/default color. |
I think the |
closes #2431
refs #2510
Implements @RedSparr0w 's suggestion from #2431 (comment)
light logo:
dark logo:
medium logo:
if we're overriding colorA, meh.. the user can work it out 🤷♂️