Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution, great work! The stopwatch is fully functional and meets all the core requirements of the task, including the animations and the speed-up modifier. The SCSS structure with variables and BEM is also well-implemented.
While your submission is approved, please take note of the feedback for future improvements. Specifically, remember to apply box-sizing: border-box globally as per the code style guide for more predictable styling. Also, you can make your CSS cleaner by removing a few redundant properties that were pointed out, such as margin: 0 auto; on absolutely positioned elements.
Overall, this is a solid implementation. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| justify-content: center; | ||
| height: 100vh; | ||
| width: 100vw; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
According to the code style rules, box-sizing: border-box should be applied to all elements. It's common practice to apply this rule globally using a universal selector (*) at the beginning of your base styles, rather than just on a single element.
src/styles/stopwatch.scss
Outdated
| .stopwatch { | ||
| height: $stopwatch-size; | ||
| width: $stopwatch-size; | ||
| margin: 0 auto; |
There was a problem hiding this comment.
This rule is redundant for centering the stopwatch. The parent element .main is already centering it horizontally and vertically using flexbox properties (justify-content: center and align-items: center).
src/styles/stopwatch.scss
Outdated
| position: relative; | ||
|
|
||
| &__minutes-hand { | ||
| margin: 0 auto; |
There was a problem hiding this comment.
Since this element is absolutely positioned and already centered horizontally with left: 50% and transform, this margin property has no effect and can be removed.
src/styles/stopwatch.scss
Outdated
| position: absolute; | ||
| bottom: $hand-minutes-position-top; | ||
| left: $hand-minutes-position-left; | ||
| transform: translateX(-50%); |
There was a problem hiding this comment.
This transform property is immediately overridden by the animation which also defines transform in its keyframes. Therefore, this line is redundant and can be removed without affecting the result.
src/styles/stopwatch.scss
Outdated
| } | ||
|
|
||
| &__seconds-hand { | ||
| margin: 0 auto; |
There was a problem hiding this comment.
Since this element is absolutely positioned and already centered horizontally with left: 50% and transform, this margin property has no effect and can be removed.
src/styles/stopwatch.scss
Outdated
| position: absolute; | ||
| bottom: $hand-seconds-position-top; | ||
| left: $hand-seconds-position-left; | ||
| transform: translateX(-50%); |
There was a problem hiding this comment.
This transform property is immediately overridden by the animation which also defines transform in its keyframes. Therefore, this line is redundant and can be removed without affecting the result.
src/styles/stopwatch.scss
Outdated
| } | ||
|
|
||
| &__center { | ||
| margin: 0 auto; |
There was a problem hiding this comment.
Since this element is absolutely positioned and already centered horizontally with left: 50% and transform, this margin property has no effect and can be removed.
2 participants
DEMO LINK
Keyframes implemented using from/to + transform with rotate property
Stopwatch is centered and has the correct arrows size
All
Typical Mistakesfrom theBEMlesson theory are checked.Code follows all the Code Style Rules ❗️