Executando verificação de segurança...
3

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.

2

Implementei utilizando suas dicas e cara realmente ficou muito melhor...

realmente trocar os buttons por radio faz mais sentido, e a forma de trocar as cores utilizando o data como algumas pessoas disseram e voce tambem ficou mais limpo o codigo

Tem alguma dica para pegar essa "manha" ou realmente é com o tempo e a experiencia mesmo?

2

Tem coisa que só vem com o tempo mesmo.

Mas algo que ajuda é tentar conhecer o que já existe. Existem trocentos elementos HTML, por exemplo, cada um mais indicado para determinada situação. Percebo que hoje em dia há um certo abuso em usar sempre a mesma meia dúzia de elementos e "compensar" no JavaScript. Sendo que muitas vezes já existe um elemento que faz o que vc precisa - exemplo.

Os tutoriais da MDN ajudam bastante nesse sentido, são bem completos e atualizados.

No seu caso específico, perceba que tem muito código repetitivo: tanto no botão branco quanto no preto, a maior parte é igual, só mudando alguns detalhes. Isso é um forte indício de que poderia ser melhorado (repetição por si só nem sempre é ruim, mas geralmente indica que há espaço pra melhorias).