-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat(the-cart): add TheCart component #248
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boa noite @Raullages ,
Fiquei com a impressão que você só moveu os arquivos do componente praticamente, na verdade a refatoração passa por fixes no code style também e se conveniente uma revisão da API do componente (props, slots, events).
Além disto tem um ponto com um erro mais grave no trecho em que você importa um script interno do storefront-app
, acho que o ideal na verdade seria torná-lo uma prop no TheCart
.
Também notei umas quebras em identação e temos arquivos conflitantes agora porque recentemente eu também editei algumas coisas no mesmo componente, pode resolver os conflitos por favor?
Qualquer dúvida ou discordância só me dar um toque...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Raullages tenho mais algumas considerações sobre code style e convenções da API, mas o principal problema aqui é que faltou mover umas coisas que atualmente estão no EcCart
, na sua refatoração perdemos umas features e fixes recentes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Melhorou bem @Raullages , mas comentei mais umas convenções e de novo (o mais importante) seu componente está desatualizado com o EcCart
atual (no master
), então perderíamos alguns feats/fixes na refatoração, o que não é legal 😅
Dá uma olhada lá de novo por favor, e nos comentários que eu deixei, valeu 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valeu @Raullages ,
Só comentei uns trecho que achei desnecessários e não entendi porque você colocou e tenho um problema com o nome que você escolheu canApplyCoupon
porque na verdade o sentido é justamente o contrário, quando canApplyCoupon
("pode aplicar o desconto") está true é justando quando o cupon não é aplicado (porque já foi), então o nome atual tá indicando o contrário do que a variávei é na verdade, aí fica confuso 😕
Mas são só minors agora 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agora sim @Raullages, valeu 🎆
Related issue
#182