-
Notifications
You must be signed in to change notification settings - Fork 44
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
Issue 26 #50
Issue 26 #50
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.
Oque é isso?
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.
subiu errado, não era p ter subido, tá com o link do figma que foi retirado só.
src/components/PetRegist/Ellipse.svg
Outdated
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.
Porque temos duas imagens do "Ellipse"?
Por isso se chama Ellipse?
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.
Eu exportei como SVG do figma do petdex, e lá está com o nome de selection ellipse:
acabei por colocar esse nome.
a despeito dos 2 arquivos, um é para desktop e outro para mobile, ao tentar estilizar ele no mobile, uma parte dele se ocultava caso eu desse um Width do tamanho necessário. Então a solução que achei foi duplicar o arquivo e mudar direto no código do SVG o tamanho.
src/components/PetRegist/Bars.svg
Outdated
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.
Isso não pode ser uma imagem, tem q ser feito no HTML/SVG raw pra fazer animação
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.
é um svg, exportei direto do figma do petdex.
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.
Isso aqui não existe, vc fez em css
? A gente usa scss
não css
.
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.
eu fiz em sccs.
foi uma extensão minha que criou esse .css, vou tirar ele de jogo, tinha esquecido.
* { | ||
box-sizing: border-box; | ||
width: 100%; | ||
} | ||
|
||
html { | ||
font-size: 10px; | ||
} |
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.
Vc não pode fazer isso num arquivo interno de css
, alterar o html
e usar *
é muito errado, precisa ajustar
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.
blz
position: absolute; | ||
top: -80px; | ||
z-index: -1; | ||
}/*# sourceMappingURL=index.css.map */ |
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.
Isso é um sourcemap? Pq tem um sourcemap no seu componente?
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.
po esse sourcemap eu não adicionei ai não, que brisa é essa
src/components/PetRegist/index.scss
Outdated
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.
Precisamos falar sobre isso
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.
bah, blz
…CSS e ajeitando bug do prettier fix devhatt#26
appearance: none; | ||
} | ||
} | ||
} |
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.
EOF
@@ -0,0 +1,48 @@ | |||
/* eslint-disable prettier/prettier */ |
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.
Why?
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.
prettier e eslint estão dando conflito de identação, precisei desativar um para que não desse erro. Cheguei até a falar com o Pilu e Diogo sobre esse erro ai, e te mandei msg um dia no discord mostrando o erro, no fim ele era ocasionado por conta de conflito de identação
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.
Não tinha raparado antes, mas pq o arquivo se chama PetRegist?
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.
Como ele vai fazer parte do cadastro de pets e ainda não existia nenhum arquivo, pensei em já deixar o nome para guiar o que esse componente faz parte.
setValue() { | ||
this.selected.get('weight').textContent = this.selected.get('rangeSlider').value; | ||
}, | ||
weight() { |
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.
mudar para weightChange
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.
ok
this.selected.get('weight').textContent = this.selected.get('rangeSlider').value; | ||
}, | ||
weight() { | ||
this.emit('weight'); |
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.
o nome do evento muda pra weight:change
|
||
align-items:center; | ||
|
||
padding-right: 85px; |
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.
usar rem
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.
bah não tinha visto que eu coloquei em px
padding-right: 85px; | ||
|
||
position:absolute; | ||
top: -75px; |
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.
Usar %
, no max usar outra coisa, a menos que vc me justifique o porque precisa de -75px
de top
|
||
position:absolute; | ||
top: -75px; | ||
z-index: -1; |
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.
What? Why?
.slider__input{ | ||
width: 600px; | ||
|
||
margin-top: -45px; |
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.
Identação
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.
Ta usando em alguns momentos identação de 4 espaçoes, use apenas 2
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.
Ta usando em alguns momentos identação de 4 espaçoes, use apenas 2
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.
Preciso que se atente mais na parte de estilos, principalmente em:
- Buscar soluções com melhor compatibilidade entre browsers
- Evitar repetição de propriedade
- Evitar "resolver um problema que vc mesmo causou", muito do seu CSS parece ser gambiarras que vc precisou fazer pq usou muita coisa que não deve ser usada ou alguma coisa que estragou a outra saca?
- Se atentar ao nosso padrão de unidades e como usamos https://www.linkedin.com/pulse/unidades-css-um-guia-completo-alexandre-gomes/
} | ||
|
||
.slider__input{ | ||
width: 600px; |
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.
usar %
, largura em px
é um crime hediondo
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.
OK!
outline: none; | ||
} | ||
|
||
&::-webkit-slider-runnable-track { |
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.
Isso não é muito bem suportado, precisamos talvez conversar sobre o pq isso é necessario e quais estrategias podemos usar
cursor: pointer; | ||
} | ||
|
||
&::-webkit-slider-thumb { |
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.
Isso não é muito bem suportado, precisamos talvez conversar sobre o pq isso é necessario e quais estrategias podemos usar
width: 100px; | ||
height: 100px; |
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.
Nunca never jamais
} | ||
} | ||
|
||
@media only screen and (max-width: 600px){ |
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.
A gente ainda não tem as media-query padrão?
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.
até o dia que eu subi isso não tinha não
} | ||
|
||
.slider__bars-svg{ | ||
width: 200vw; |
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.
Nop
top: 5px; | ||
z-index: -1; |
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.
Porqueeee
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.
p resolver a questão do bars-svg fica atrás na position absolute, mt errado?
.slider__input{ | ||
width: 80vw; | ||
|
||
margin-top: 15px; |
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.
identação
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.
Ale, todas as partes de identação do Sass foram colocadas pelas extensões, eu deixo tudo mt próximo um do outro, com no máximo 1 espaço de linhas.
} | ||
|
||
.slider__input{ | ||
width: 80vw; |
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.
Não
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.
Tive a impressão de que no fim desse arquivo vc duplicou alguns estilos, isso aconteceu de fato?
PR fechado decorrente de inatividade |
Closes #26
Adicionando range-slider como um componente dinâmico para futuramente setar peso do animal.