Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a Size Selector Component #265

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

JonasGz
Copy link
Contributor

@JonasGz JonasGz commented Jun 13, 2024

Closes #23

Feature

Criação do componente de seleção de peso.

Visual evidences 🖼️

DESKTOP
desktop

MOBILE
mobile

plaay

Checklist
  • [ x ] Issue linked
  • [ x ] Build working correctly
  • Tests created
Additional info N/A

src/components/SizeSelector/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Alecell Alecell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temos um problema de comportamento, precisamos que quando um size for selecionado ele seja animado para o centro e ai fique em destaque, da forma que está agora é um problema de usabilidade

Essa sessão aqui é um bom exemplo, porém precisamos que a troca de size tambem seja animada alem da troca de posição
https://www.medcel.com.br/serie
image

src/components/SizeSelector/index.js Show resolved Hide resolved
@JonasGz JonasGz force-pushed the issue-23 branch 2 times, most recently from b969706 to f013e61 Compare June 15, 2024 13:11
@Alecell Alecell mentioned this pull request Jun 19, 2024
3 tasks
@JonasGz JonasGz force-pushed the issue-23 branch 4 times, most recently from 955eb5b to 524dce6 Compare July 2, 2024 21:23
@JonasGz
Copy link
Contributor Author

JonasGz commented Jul 2, 2024

Temos um problema de comportamento, precisamos que quando um size for selecionado ele seja animado para o centro e ai fique em destaque, da forma que está agora é um problema de usabilidade

Essa sessão aqui é um bom exemplo, porém precisamos que a troca de size tambem seja animada alem da troca de posição https://www.medcel.com.br/serie image

refiz todo o componente e agora parece estar adequado para o que queremos.

src/components/SizeSelector/index.scss Show resolved Hide resolved
src/components/SizeSelector/index.scss Show resolved Hide resolved
src/components/SizeSelector/index.scss Show resolved Hide resolved
Comment on lines +90 to +107
if (event.key === 'ArrowRight' || event.key === 'ArrowDown') {
next = card.nextElementSibling;
if (next) {
nextIndex = this.$cards.indexOf(next);
next.focus();
this.selectCard(next, nextIndex);
this.getCardActiveEvent('keydown', next, nextIndex);
}
}
if (event.key === 'ArrowLeft' || event.key === 'ArrowUp') {
prev = card.previousElementSibling;
if (prev) {
prevIndex = this.$cards.indexOf(prev);
prev.focus();
this.selectCard(prev, prevIndex);
this.getCardActiveEvent('keydown', prev, prevIndex);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seria legal separar isso em funções, acredito inclusive que vc consegue fazer apenas 1 função que lida com o "proximo" item, passando os parametros corretos ele sabe lidar se é um item seguinte ou um item anterior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consegui reduzir pra uma função apenas mas nao sei se daria p melhorar, vou conversar ctg

Comment on lines +116 to +119
if (index === 0)
this.$sizeList.classList.add(
'container-size-selector__size-list--active-padding',
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oq isso faz?

Pq precisamos disso?

Outra coisa, quando vc tem um conteudo de um if mais complexo sempre abra o bloco do if, isso q vc fez funciona, mas prejudiuca muito a leitura

Copy link
Contributor Author

@JonasGz JonasGz Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o motivo disso é pelo seguinte:
eu preciso adicionar um padding nas laterais para ter espaço para o primeiro card e o último scrollarem para o centro.
no entanto, quando eu adiciono o padding da lateral esquerda na inicialização, os cards já iniciam totalmente para a direita colocando o primeiro card ao centro, que é diferente do comportamento que esperamos de ter o card do meio no centro e não o primeiro.
Para resolver isso eu coloquei para o componente iniciar sem padding a esquerda e adicionar apenas quando eu quiser que o primeiro card vá para o centro que é clicando no mesmo.

adicionei o bloco nos if

Comment on lines +121 to +125
const cardRect = card.getBoundingClientRect();
const listRect = this.$sizeList.getBoundingClientRect();
const cardCenter = cardRect.left + cardRect.width / 2;
const listCenter = listRect.left + listRect.width / 2;
const offset = cardCenter - listCenter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To com a sensaçào de que esse não é o melhor modo de fazer...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vou conversar ctg

element.setAttribute('aria-checked', 'true');
},

getCardActiveEvent(eventName, card, index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O nome dessa função ta meio ruim, deveria ser trigger events ou algo assim, get usamos quando pegamos algo, nesse caso não estamos pegando nada hm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poderia ser emitCardEvent?

}
},

getCardActive() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Esse nome tambem não me parece correto. No caso vc ta retornando, mas seria getActiveCard

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrigi para getActiveCard e to pensando se da pra usar outro nome

src/components/SizeSelector/index.js Show resolved Hide resolved
src/components/SizeSelector/index.js Show resolved Hide resolved
feat: add responsive

chore: changed imgs to template sting

fix: translation bug mobile

fix: add aria-checked event

feat: create new sizeSelector

feat: create methods and add navigation with keyboard

feat: finish component

fix: remove css noPet

fix: remove justify-content: center
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Desenvolvimento de Componente Slider com Controle de Navegação
3 participants