Skip to content

Develop#4161

Open
ZloyHoma wants to merge 4 commits into
mate-academy:masterfrom
ZloyHoma:develop
Open

Develop#4161
ZloyHoma wants to merge 4 commits into
mate-academy:masterfrom
ZloyHoma:develop

Conversation

@ZloyHoma

Copy link
Copy Markdown
  • DEMO LINK

  • Keyframes implemented using from/to + transform with rotate property

  • Stopwatch is centered and has the correct arrows size

  • All Typical Mistakes from the BEM lesson theory are checked.

  • Code follows all the Code Style Rules ❗️

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because a few core task requirements and checklist items are still not met. The seconds hand currently uses steps(60) via the shared hand-animation mixin (see src/styles/index.scss and src/styles/utils/variables.scss), but the task explicitly requires an even-speed (linear) animation for seconds and steps(60) only for the minutes hand, so you need to move animation-timing-function out of the common mixin and set linear on .stopwatch__seconds-hand and steps(60) on .stopwatch__minutes-hand separately. The @keyframes clock definition uses 0%/100%, while the checklist demands from/to keyframes with transform: rotate(...), so please rewrite it as:

@keyframes clock {
  from { transform: rotate(0deg); }
  to { transform: rotate(360deg); }
}

The stopwatch is also not truly centered vertically because .stopwatch is positioned with position: absolute; top: 9vmin; (in both index.scss and variables.scss), which pulls it out of the flex centering on body; remove the absolute positioning and top offset so the flexbox centering can place it exactly in the middle of the viewport. As a minor point, double‑check your BEM usage for .stopwatch--speed-up .stopwatch__minutes-hand / .stopwatch__seconds-hand against your course’s “Typical Mistakes” section; if needed, adjust the SCSS nesting (for example, &--speed-up &__seconds-hand) to fully comply. Once you fix the animation timing for each hand, switch keyframes to from/to, and let flexbox handle centering, your solution will match the functional and checklist requirements very well.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/index.scss Outdated
Comment on lines +13 to +14
position: absolute;
top: 9vmin;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the stopwatch centering requirement: using position: absolute; with top: 9vmin; moves the stopwatch away from true vertical center. Since body is already a flex container with align-items: center; and justify-content: center;, you can rely on flexbox and remove the absolute positioning/offset.

Comment thread src/styles/index.scss
Comment on lines +29 to +36
@keyframes clock {
0% {
transform: rotate(0deg);
}

100% {
transform: rotate(360deg);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checklist item requires keyframes to be implemented using from/to with transform: rotate(...). Here you use 0% and 100%. Update this to from { transform: rotate(0deg); } and to { transform: rotate(360deg); } to match the checklist.

Comment thread src/styles/index.scss
Comment on lines +12 to +15
justify-content: center;
position: absolute;
top: 9vmin;
height: $watch-size;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the description, the seconds hand must have an even-speed animation without acceleration or deceleration (linear), while only the minutes hand should use steps(60). Putting animation-timing-function: steps(60); in this shared mixin applies it to both hands and violates that requirement. Consider removing the timing function from the mixin and setting linear on the seconds hand and steps(60) on the minutes hand separately.

Comment on lines +10 to +14
$seconds-hand-height: 38vmin;

@mixin hand-animation($duration) {
animation-name: clock;
animation-duration: $duration;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stopwatch element is positioned absolutely and offset with top: 9vmin;, which breaks true vertical centering. The task requires the stopwatch to be centered vertically and horizontally; since the body is already a flex container, consider removing position: absolute; and the top offset so the flex centering can work correctly.

Comment on lines +12 to +17
@mixin hand-animation($duration) {
animation-name: clock;
animation-duration: $duration;
animation-timing-function: steps(60);
animation-iteration-count: infinite;
transform-origin: bottom center;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your mixin hardcodes animation-timing-function: steps(60); for all hands. According to the task, only the minutes hand should use steps(60); the seconds hand must have even speed (linear). This violates the requirement and checklist about different animations for each hand; consider parameterizing the timing function or using separate mixins for seconds and minutes.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution because it fully meets the functional and visual requirements of the stopwatch task. The stopwatch block and stopwatch--speed-up modifier are correctly implemented in HTML, and the layout is properly centered using flexbox on body without conflicting positioning. All specified dimensions, colors, and shapes (80vmin circular watch, 1vmin dotted border, 5vmin orange center dot, correctly sized green seconds and blue minutes hands) match the requirements. The CSS-only animations are set up correctly: the seconds hand rotates smoothly with a linear 60s (and 10s in speed-up mode) animation, and the minutes hand uses steps(60) with 3600s (and 600s in speed-up mode), both looping infinitely. Your BEM usage for .stopwatch, its elements, and the stopwatch--speed-up modifier is consistent and avoids the common mistakes mentioned in the theory. Overall, there are no critical blockers or checklist violations remaining, and this is a solid, clean implementation—well done.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants