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

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

Boa noite galera!

Pessoal estou apredendo programação afim de entrar na área, como não saiu nenhuma vaga vou fazendo os freelas que aparece.

Como não tenho experiencia em aplicações que são usadas em empresas queria tirar uma duvida com vocês, como sei se meu código está bom ou no padrão que vocês devs usam nas empresas.

Meu foco é Front end, então para contextualizar segue essa landing page que fiz para um cliente divulgar seu produto, a ideia é que quando o usuário escolhe uma cor a foto do produto muda para aquela cor.

Basicamente criei buttons devido a acessibilidade, e fiz um add.eventListener para cada um deles e troco o atributo src da tag img no meu html.

codigo resumido:

const btnBlack = document.querySelector("#black")
const btnBlue = document.querySelector("#blue")
const btnOrange = document.querySelector("#orange")
const btnPink = document.querySelector("#pink")
const btnGrey = document.querySelector("#grey")
const btnGold = document.querySelector("#gold")
const productImg = document.querySelector(".product img")

function changeImg(path) {
 
    productImg.setAttribute("src", path)
    productImg.classList.add("fadeJS")
    productImg.style.filter = "grayscale(0)"

    setTimeout(() => {
        productImg.classList.remove("fadeJS")
    }, 200);
}

// funcao que executa a troca de imagens, repare que o path eh uma parametro

function changeImg(path) {
 
    productImg.setAttribute("src", path)
    productImg.classList.add("fadeJS")
    productImg.style.filter = "grayscale(0)"

    setTimeout(() => {
        productImg.classList.remove("fadeJS")
    }, 200);
}

// opcoes de cores quando usuario clicka na cor branca
btnWhite.addEventListener("click", () => {
    changeImg(path = "/public/orange.opti.webp")
    productImg.style.filter = "grayscale(1)"

    btnBlue.addEventListener("click", () => {
        changeImg(path = "/public/azul-off.opti.webp")
        btnWhite.focus()
    })

    btnOrange.addEventListener("click", () => {
        changeImg(path = "/public/orange.opti.webp")
        btnWhite.focus()
    })

    btnPink.addEventListener("click", () => {
        changeImg(path = "/public/pink.opti.webp")
        btnWhite.focus()
    })

    btnGrey.addEventListener("click", () => {
        changeImg(path = "/public/grey.opti.webp")
        btnWhite.focus()
    })

    btnGold.addEventListener("click", () => {
        changeImg(path = "/public/gold.opti.webp")
        btnWhite.focus()
    })
})

// opcoes de cores quando usuario clicka na cor preta
btnBlack.addEventListener("click", () => {
    changeImg(path = "/public/orange-dark.opti.webp")
    productImg.style.filter = "grayscale(1)"

    btnBlue.addEventListener("click", () => {
        changeImg(path = "/public/azul-off-dark.opti.webp")
        btnBlack.focus()
    })

    btnOrange.addEventListener("click", () => {
        changeImg(path = "/public/orange-dark.opti.webp")
        btnBlack.focus()
    })

    btnPink.addEventListener("click", () => {
        changeImg(path = "/public/pink-dark.opti.webp")
        btnBlack.focus()
    })

    btnGrey.addEventListener("click", () => {
        changeImg(path = "/public/grey-dark.opti.webp")
        btnBlack.focus()
    })

    btnGold.addEventListener("click", () => {
        changeImg(path = "/public/gold-dark.opti.webp")
        btnBlack.focus()
    })
})

Funcionou mas eai esse código é profissional ou gambiarra?

Site pix em 3D animacao

link do site: https://pix.em3de.com/
link do codigo: https://github.com/Dener-Garcia/pix.em3d/blob/production/src/app.js

ps: não tenho frescuras podem mandar as respostas do jeito que quiserem

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

2

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!

1

achei muito interessante essa abordagem com data.
é disso que to falando ficaria mais enxuto o codigo, esse tipo de filling que preciso pegar sabe

1

O problema é que cada vez que clicar no botão branco ou preto, um novo listener é adicionado a todos os outros botões.

E como addEventListener é acumulativo, no fim cada botão pode acabar ficando com vários listeners, o que é desnecessário.

Eu detalho melhor esse problema no meu comentário.

Mas a ideia de usar dataset é boa, tanto que acabei usando também - de uma forma diferente, embora a ideia geral seja similar.

1

So repassando o codigo ficou da seguinte forma:

HTML

            <h3>Personalize sua placa</h3>
            <div class="product-tune">
                <label>Placa</label>
                <div class="tune-colors">
                    <input type="radio" name="tone" aria-label="Cor da placa Branca" value="white" >
                    <input type="radio" name="tone" aria-label="Cor da placa Preto" value="black">
                </div>
                <label>Detalhes</label>
                <div class="tune-colors">
                    <input type="radio" name="color" aria-label="Azul" data-tone-white="/public/azul-off.opti.webp" data-tone-black="/public/azul-off-dark.opti.webp" value="blue">
                    <input type="radio" name="color" aria-label="Laranja" data-tone-white="/public/orange.opti.webp" data-tone-black="/public/orange-dark.opti.webp" value="orange">
                    <input type="radio" name="color" aria-label="Rosa" data-tone-white="/public/pink.opti.webp" data-tone-black="/public/pink-dark.opti.webp" value="pink">
                    <input type="radio" name="color" aria-label="Cinza" data-tone-white="/public/grey.opti.webp" data-tone-black="/public/grey-dark.opti.webp" value="grey">
                    <input type="radio" name="color" aria-label="Dourado" data-tone-white="/public/gold.opti.webp" data-tone-black="/public/gold-dark.opti.webp" value="gold">
                </div>

meu javascript

const btnColors = document.querySelectorAll(".tune-colors input[name='color']");
const productImg = document.querySelector(".product img")

function changeImg(pathImg, frameColor, boardColor, ) {
    productImg.setAttribute("src", pathImg)
    productImg.style.filter = "grayscale(0)"
    productImg.setAttribute("alt", "Placa com QR Code" + " " + frameColor + " " + "personalizada" + " " + "com detalhes em" + " " + boardColor)
    

}

for (let detailColor of btnColors) {

    detailColor.addEventListener('change', function (event) {

        let boardColor = document.querySelector("input[name='tone']:checked");
        if (!boardColor) {
            alert('Ops, Escolha a cor da placa antes 😉');

            return;
        }

        switch (boardColor.value) {
            case "white":
                changeImg(
                    pathImg = detailColor.dataset.toneWhite,
                    frameColor = "Branca" ,
                    boardColor = detailColor.ariaLabel
                    )
                break;
            case "black":
                changeImg(
                    pathImg = detailColor.dataset.toneBlack,
                    frameColor = "Preta" ,
                    boardColor = detailColor.ariaLabel
                    )
            default:
                break;
        }
    });
}
1

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! :)

1

obrigado por responder.

cara nao tenho a menor ideia de como faria isso com um add.EventListener
mas o @lesnock deu uma. oa sugestão complementando que vx disse agora fez sentido

2
1