< > Código se funcionou está certo? </ > Resolvido: Trocando imagens com radio e javascript

Respondendo ao título: de forma geral, funcionar é diferente de estar certo. Tem vezes que até "funciona", mas não está feito de um jeito bom. Ou só funciona por coincidência, ou apenas no caso específico, ou às vezes o problema só ocorre nos casos não testados, etc.

No seu caso, cada vez que clicar em um dos botões branco ou preto, será adicionado outro listener nos demais botões. O problema é que addEventListener é acumulativo.

Para ilustrar o problema, vou usar este HTML (deixei apenas as cores azul e laranja, para simplificar):

<button id="white">branco</button>
<button id="black">preto</button>
<button id="blue">azul</button>
<button id="orange">laranja</button>

E um JavaScript similar ao seu:

const btnWhite = document.querySelector("#white");
const btnBlack = document.querySelector("#black");
const btnBlue = document.querySelector("#blue");
const btnOrange = document.querySelector("#orange");

btnWhite.addEventListener("click", () => {
    console.log('clicou branco');

    btnBlue.addEventListener("click", () => {
        console.log('clicou azul claro');
    });
    btnOrange.addEventListener("click", () => {
        console.log('clicou laranja claro');
    });
});

btnBlack.addEventListener("click", () => {
    console.log('clicou preto');

    btnBlue.addEventListener("click", () => {
        console.log('clicou azul escuro');
    });
    btnOrange.addEventListener("click", () => {
        console.log('clicou laranja escuro');
    });
});

Ou seja, com a mesma lógica do seu: ao clicar no botão preto ou branco, ele adiciona outro listener nos demais.

Primeiro cliquei no botão branco, ele imprimiu:

clicou branco

Depois cliquei no azul, imprimiu:

clicou azul claro

Até aqui tudo bem. Depois cliquei no botão preto, imprimiu:

clicou preto

Cliquei no azul e agora aparece o problema, pois imprimiu:

clicou azul claro
clicou azul escuro

Pois é, ele executou primeiro o listener adicionado pelo botão branco, e depois o adicionado pelo botão preto.

Novamente cliquei no botão branco e depois no azul, o resultado foi:

clicou branco
clicou azul claro
clicou azul escuro
clicou azul claro

E se clicar no laranja:

clicou laranja claro
clicou laranja escuro
clicou laranja claro

A cada clique nos botões branco ou preto, um novo listener é adicionado aos botões azul e laranja. O código só "funciona" porque os listeners executam na ordem em que foram adicionados, então o último sempre é do mais recente entre o branco e o preto.

Mas eu não considero isso certo, porque não há motivo para ficar executando todos esses listeners. Só interessa que ele mostre a informação de acordo com as cores selecionadas. Mesmo que "funcione" e na maioria dos casos não dê problema (pelo menos nada perceptível pelo usuário), ainda sim é um code smell: algo que "não cheira bem", e que no longo prazo pode acabar no problema das janelas quebradas.

No seu site, fiz um teste clicando no branco e no preto, de forma alternada, 100 vezes em cada. Ao clicar nas outras cores, começou a ter um pequeno atraso (cerca de 1 segundo), porque está executando todos os listeners acumulados. Claro, provavelmente ninguém vai clicar tantas vezes assim, mas enfim, de qualquer forma, não acho que é uma boa solução ficar adicionando listeners toda hora.

Inclusive, acho até que usar button não é a melhor escolha.

Entre o preto e o branco, somente um deles está ativo, o mesmo vale para o outro conjunto de cores (somente uma dentre azul, laranja, etc, será escolhida por vez).

Sendo assim, vc tem dois grupos de opções, nos quais somente um dos valores está selecionado - ao escolher entre preto ou branco, somente um deles está "ativo", o mesmo vale para as outras cores - somente uma delas está selecionada a cada momento.

Então, semanticamente falando, faria mais sentido ter dois grupos de input type="radio". Algo assim (deixei apenas duas cores, mas basta adicionar mais campos para ter todas as cores):

<!-- branco e preto -->
<input type="radio" name="tom" value="claro">
<input type="radio" name="tom" value="escuro">

<!-- demais cores -->
<input type="radio" name="cor" value="azul" data-claro="/public/azul-off.opti.webp" data-escuro="/public/azul-off-dark.opti.webp">
<input type="radio" name="cor" value="laranja" data-claro="/public/orange-off.opti.webp" data-escuro="/public/orange-off-dark.opti.webp">

Os dois primeiros input's referem-se ao branco e preto, os demais são as cores azul e laranja. Estas duas também possuem campos no dataset referentes às URL's para as versões clara e escura.

O JavaScript fica assim:

// para cada cor
for (const cor of document.querySelectorAll("input[name='cor']")) {
    // sempre que o valor mudar (selecionar alguma cor)
    cor.addEventListener('change', function(event) {
        // verifica o tom (branco ou preto) que está selecionado
        const tom = document.querySelector("input[name='tom']:checked");
        if (!tom) { // nenhum tom está selecionado
            alert('escolha um tom - branco ou preto');
            return;
        }
        // target é o elemento referente à cor que foi selecionada
        // e o value do respectivo tom pega a URL do dataset
        changeImg(event.target.dataset[tom.value]);
    });
}

Agora, se escolher a cor azul ou laranja, ele verifica se foi escolhido preto ou branco, e pega o respectivo caminho da imagem (que está no dataset de cada elemento).

Se precisar adicionar mais cores (rosa, cinza, etc), basta colocar mais input's com os respectivos datasets, sem precisar mudar o JavaScript.

E para estilizar, o CSS ficaria assim (apenas para mostrar as cores, o restante vc ajusta de acordo com o seu layout):

input[type='radio']:after {
    width: 15px;
    height: 15px;
    border-radius: 15px;
    top: -2px;
    left: -1px;
    position: relative;
    background-color: white;
    content: '';
    display: inline-block;
    visibility: visible;
    border: 2px solid black;
}
input[type='radio']:checked {
    transform: scale(1.2);
}
input[value='claro']:after {
    background-color: white;
}
input[value='escuro']:after {
    background-color: black;
}
input[value='azul']:after {
    background-color: blue;
}
input[value='laranja']:after {
    background-color: orange;
}

A primeira regra é para que cada radio button ser um círculo (ajuste os tamanhos de acordo com o seu layout). A segunda regra é para aumentar o tamanho quando é selecionado. Depois, coloco a cor específica para cada um.

É só a ideia básica, depois vc ajusta para o seu layout.

Sei que a maioria optou por não alterar a estrutura da página, mas eu não vi muita vantagem em manter tantos botões. O radio me parece o mais adequado, já que a ideia dele é justamente ter grupos de valores nos quais somente um é selecionado por vez.

Cara, seu código funciona e é claro no que faz. E acho que isso é o que mais importa. No entanto, eu sempre tento pensar em deixar código de forma que a manutenção no futuro seja fácil. Por exemplo, ao invés de criar um id para cada botão, você poderia criar um atributo igual para todos os botões e outro atributo com a url da imagem. Ficaria mais ou menos assim:

No HTML criamos o atributo data-button para cada botão e adicionamos outro atributodata-image-url que armazena a url da imagem que irá aparecer quando aquele botão for clicado:

<button data-button data-image-url=¨/public/azul-off.opti.webp¨>Blue</button>
<button data-button data-image-url=¨/public/orange-off.opti.webp¨>Orange</button>
<button data-button data-image-url=¨/public/pink-off.opti.webp¨>Pink</button>
<!-- ... -->

E aí no Javascript você apenas cria um loop que vai automaticamente adicionar os event listeners para cada botão:

document.querySelectorAll('[data-button]').forEach(button => {
    button.addEventListener('click', () => {
        changeImage(button.dataset.imageUrl)
    })
}

Desta forma, se no futuro você precisar adicionar uma nova cor, a única coisa que você vai precisar fazer é adicionar um novo botão com a URL da nova imagem.

Se você nunca utilizou atributos do tipo data talvez este código esteja um pouco confuso para você. Link para você entender melhor sobre os data attributes.

Espero ter ajudado!

Vc pode usar uma mesma class e fazer a troca com apenas 1 evento. Mas no seu caso não faz muita diferença em performance nem nada!

Eu consigo entender bem o código. Pra mim ta bom! :)