feat: create a button top #242
Conversation
✅ Deploy Preview for diciotech ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
esterfania
force-pushed
the
feat/button-top
branch
from
92a3046 to
d46e292
Compare
esterfania
force-pushed
the
feat/button-top
branch
from
d46e292 to
81564c5
Compare
| @@ -16,6 +16,7 @@ | |||
| <link rel="preconnect" href="https://fonts.googleapis.com" /> | |||
| <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin /> | |||
| <link rel="stylesheet" href="assets/css/style.css" /> | |||
| <link rel="stylesheet" href="assets/css/button_up.css" /> | |||
There was a problem hiding this comment.
É melhor adicionar o button_up no sass ao invés de aqui, isso evita ficar criando vários arquivos css sem necessidade, assim
diciotech/assets/sass/style.sass
Line 5 in b094b87
There was a problem hiding this comment.
Acho que o nome button_top ou até scroll_to_top é melhor. Ou talvez até adicionar isso ao assets/sass/style.sass já que é pouca coisa, não?
|
|
||
| .button-up | ||
| background-color: $primary-500 | ||
| color: $white |
There was a problem hiding this comment.
Ao invés de usar $white e $primary-500 diretamente, é melhor já fazer pensando em possíveis valores diferentes para os temas. Exemplo do que foi feito com a janela de cookies:
Cor pro tema claro:
Cor pro tema escuro:
diciotech/assets/sass/dark_theme.sass
Line 8 in b094b87
Como usar a variável:
diciotech/assets/sass/cookies.sass
Line 5 in b094b87
| @@ -83,9 +84,14 @@ <h2 class="header__subtitle"> | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <button class="button-up"> | |||
| Topo | |||
There was a problem hiding this comment.
Só uma opinião, será que é necessária a palavra Topo? Porque eu acho que só a setinha é suficiente, e caso a gente adicione outras línguas é mais um lugar pra adicionar um termo traduzido que acho que não é essencial
There was a problem hiding this comment.
@george-gca @esterfania minha única preocupação em não ter nenhum texto é que incluir a palavra "topo" pode ser mais claro para usuários menos familiarizados com ícones, mas acredito que como essa setinha pra voltar ao topo da página é bem comum, pode não ser um grande problema.
|
Obrigada pela revisão @george-gca! |
There was a problem hiding this comment.
@esterfania obrigada pela sua contribuição 🫰🏻 fico no aguardo nas respostas dos comentários
| @@ -83,9 +84,14 @@ <h2 class="header__subtitle"> | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <button class="button-up"> | |||
| Topo | |||
There was a problem hiding this comment.
@george-gca @esterfania minha única preocupação em não ter nenhum texto é que incluir a palavra "topo" pode ser mais claro para usuários menos familiarizados com ícones, mas acredito que como essa setinha pra voltar ao topo da página é bem comum, pode não ser um grande problema.
Descrição de PR
Issue relacionado
Motivações
A motivação dessa alteração é contribuir com open source
Informações adicionais
Essa PR adiciona um botão que retorna para o topo da página. Segue vídeo do comportamento: