-
Notifications
You must be signed in to change notification settings - Fork 0
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
Loader component #108
base: frontend
Are you sure you want to change the base?
Loader component #108
Conversation
✅ Deploy Preview for akai-guide-me ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Wow! Super robota! Szczególnie z tym SCSS! Gratulacje 🥳
Przypomniało mi się, że brakuje jeszcze Storybooka :) Jesteś w stanie dodać jeszcze tam ten komponent?
@@ -0,0 +1,87 @@ | |||
@import '../../assets/styles/abstracts/variables'; | |||
|
|||
$coloursPrimary: |
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.
Tutaj w sumie mogłabyś zmienić nazwę zmiennej na taką, która bardziej by pasowała CSSowej konwencji nazewniczej, tzn. wykorzystać kebab-case.
Czyli zamiast $coloursPrimary
to $colours-primary
|
||
$delays: 0.1s, 0.25s, 0.4s, 0.6s, 0.8s; | ||
|
||
@mixin loader-dots-colouring($colours, $delays) { |
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.
I w sumie możesz zmienić jeszcze wszędzie wystąpienia colour
na color
. Obie wersje językowo są poprawne, ale w programowaniu częściej można znaleźć tą krótszą 😉
transform: translateY(0); | ||
} | ||
25% { | ||
transform: tborderranslateY(5px); |
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.
tutaj masz chyba literówkę, wydaje mi się, że powinno być samo translateY
width: 35px; | ||
margin-right: 15px; | ||
} | ||
@keyframes loading { |
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.
Myślę, że możesz dać @keyframes
w mixin i poprzez jakiś argument definiować jak wysoko mają te kropki skakać.
Ten fragment kodu jest prawie identyczny z tym co jest w linijce 44, więc można z tego zrobić swego rodzaju funkcję :)
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.
Super robota! 🥳
@@ -0,0 +1,78 @@ | |||
@import '../../assets/styles/abstracts/variables'; |
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.
replace with @use
@@ -0,0 +1,78 @@ | |||
@import '../../assets/styles/abstracts/variables'; | |||
|
|||
$colors-primary: |
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.
Aren't these colors in variables file?
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.
There is array of colors definition. In file variables.scss
one variable stores one value. In this case array is good approach imo
align-items: center; | ||
|
||
span { | ||
height: 25px; |
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.
convert to rem or add TODO comment to dont forget to refactor in the future ;)
No description provided.