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.
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?