6
\$\begingroup\$

I've made a little colour conversion app, and below is my JavaScript.

The app converts HEX values to RGB values and vice versa, as well as using a simple colour picker to get colours and convert them to both HEX and RGB.

Here is a picture so it is easier to understand what the code does. Basically you enter a HEX or RGB value into HTML input fields, and then the converted RGB/HEX value gets printed out.

You can also choose a colour from the 'Pick A Colour' section, and the colour selected also gets converted into HEX/RGB:

Converter

Does anyone have any suggestions to shorten this?

var svgPath = document.getElementById('path');
var colorInput = document.getElementById('color-input');
var hexOutput = document.getElementById('hex-output');
var rgbOutput = document.getElementById('rgb-output');
var rgb = document.getElementById("rgb");
var rgb2 = document.getElementById("rgb2");
var rgb3 = document.getElementById("rgb3");

// HEX to RGB
function hexToRGB(hex) {
    var bigint = parseInt(hex, 16);
    var r = (bigint >> 16) & 255;
    var g = (bigint >> 8) & 255;
    var b = bigint & 255;
    return r + ', ' + g + ', ' + b;
}

// RGB to HEX
function rgbToHEX(red, green, blue) {
    var rgb = blue | (green << 8) | (red << 16);
    return '#' + (0x1000000 + rgb).toString(16).slice(1);
}

// Gets HEX input 
document.getElementById("hex").addEventListener("input", function() {
    // Sets colours for SVG, picker, converts HEX to RGB and prints out HEX
    svgPath.style.fill = '#' + this.value;
    colorInput.value = '#' + this.value;
    hexOutput.innerHTML = 'HEX: #' + this.value.toUpperCase();
    rgbOutput.innerHTML = 'RGB: rgb(' + hexToRGB(this.value) + ')';
}, false);

// Selects elements under .rgb class
var rgbInput = document.querySelectorAll('.rgb');
for(var i = 0; i < rgbInput.length; ++i){
    rgbInput[i].addEventListener("input", rgbCalc);
}

function rgbCalc() {
    // Skips to next input box if 3 characters have been entered
    if (rgb.value.length >= this.maxLength) {
        rgb2.focus();
    }
    if (rgb2.value.length >= this.maxLength) {
        rgb3.focus();
    }

    // Stores the r, g, b values
    var r = rgb.value;
    var g = rgb2.value;
    var b = rgb3.value;

    // Sets colours for SVG, picker, converts RGB to HEX and prints out RGB
    svgPath.style.fill = rgbToHEX(r, g, b);
    colorInput.value = rgbToHEX(r, g, b);
    rgbOutput.innerHTML = 'RGB: rgb(' + r + ', ' + g + ', ' + b + ')';
    hexOutput.innerHTML = 'HEX: ' + rgbToHEX(r, g, b).toUpperCase();
}

// Gets colour picker input value
colorInput.addEventListener("input", function() {
    // Sets colours for SVG, converts Hex to RGB and prints out HEX
    svgPath.style.fill = colorInput.value;
    hexOutput.innerHTML = "HEX: " + colorInput.value.toUpperCase();
    rgbOutput.innerHTML = "RGB: rgb(" + hexToRGB(colorInput.value.replace('#', ''))  + ')';
}, false);
\$\endgroup\$
1
\$\begingroup\$

Nice code, only a few comments to offer here:

.1.

var rgb = document.getElementById("rgb");
var rgb2 = document.getElementById("rgb2");
var rgb3 = document.getElementById("rgb3");

This way of keeping the r, g, and b is not the best. Consider having to keep six of them, or more. You would not do this that way. You cannot run a loop with this and this gets obvious in

    if (rgb.value.length >= this.maxLength) {
        rgb2.focus();
    }
    if (rgb2.value.length >= this.maxLength) {
        rgb3.focus();
    }

So what I would do is:

var getEl = document.getElementById;
var rgbElements = {
                   r:getEl("rgb"),
                   g:getEl("rgb2"),
                   b:getEl("rgb3")
                  }

This way you could run a loop for these.

.2. This code

document.getElementById("hex").addEventListener("input", function() {
    // Sets colours for SVG, picker, converts HEX to RGB and prints out HEX
    svgPath.style.fill = '#' + this.value;
    colorInput.value = '#' + this.value;
    hexOutput.innerHTML = 'HEX: #' + this.value.toUpperCase();
    rgbOutput.innerHTML = 'RGB: rgb(' + hexToRGB(this.value) + ')';
}, false);

should be like this one

document.getElementById("hex").addEventListener("input", function() {
    // Sets colours for SVG, picker, converts HEX to RGB and prints out HEX
    var hexVal = this.value;
    svgPath.style.fill = '#' + hexVal;
    colorInput.value = '#' + hexVal;
    hexOutput.innerHTML = 'HEX: #' + hexVal.toUpperCase();
    rgbOutput.innerHTML = 'RGB: rgb(' + hexToRGB(hexVal) + ')';
}, false);

You should not 'ask' for values from 'this' or higher scope when you can have it closer.

\$\endgroup\$
0
\$\begingroup\$

Nice app. Here are some minor nits and optional tips:

  1. Some developers will do var Element = document.getElementById; so that getting an element by ID requires less typing. This is completely up to you.

  2. Comments should explain things that are not immediately obvious from the code.

    // Stores the r, g, b values
    var r = rgb.value;
    var g = rgb2.value;
    

    This is not really a useful comment, since it adds no value to the understanding of the reader.

Otherwise, this is clear and easy to read.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.