Skip to content

Stop watch#4157

Open
Lia-k wants to merge 2 commits into
mate-academy:masterfrom
Lia-k:develop
Open

Stop watch#4157
Lia-k wants to merge 2 commits into
mate-academy:masterfrom
Lia-k:develop

Conversation

@Lia-k

@Lia-k Lia-k commented Jun 12, 2026

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 requirements are still not met and they affect the visible behavior of the stopwatch.

Critical issues that must be fixed:

  1. The hands do not start at the top (12 o’clock) as required: in @keyframes tick you currently use from { transform: translateX(-50%) rotate(0deg); }, which with your bottom: 50% and transform-origin: bottom makes the hand initially point downwards; adjust the base transform and/or add a rotation offset so that the from state visually points straight up while still making a full turn (360deg) in 60s/3600s.
  2. In the same keyframes, from uses translateX(-50%) while to uses translate(-50%); this mismatch can shift the rotation center and break the top starting/ending position, so both should use the same translate function (e.g. translateX(-50%)).
  3. Vertical centering is not guaranteed because body uses min-height: 100vh instead of height: 100vh; the task explicitly requires the stopwatch to be centered vertically and horizontally, so make the main page container a full-height flexbox (e.g. body { display: flex; justify-content: center; align-items: center; height: 100vh; }).
  4. Inside the &--speed-up modifier in src/styles/components/stopwatch.scss, you directly nest .stopwatch__hand--seconds and .stopwatch__hand--minutes; to avoid a typical BEM mistake, refactor this to modifier + element composition (for example in SCSS: &--speed-up &__hand--seconds { ... } and &--speed-up &__hand--minutes { ... }).

Once these are corrected, your existing sizes, colors, durations, and use of steps(60) and linear animations are all solid, and the solution should fully meet the task and checklist requirements—you're very close.


✨ 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/index.html
<body>
<h1>Stop watch</h1>
<div class="stopwatch stopwatch--speed-up">
<div class="stopwatch__hand stopwatch__hand--minutes"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The minutes hand animation setup is generally correct, but because of the starting-position issue in the keyframes, it doesn’t currently begin at the top as required; after you adjust the keyframes/base transform, ensure this hand still makes a full turn in 3600s with steps(60).

Comment thread src/styles/components/stopwatch.scss Outdated
transform: translateX(-50%) rotate(0deg);
}
to {
transform: translate(-50%) 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.

to { transform: translate(-50%) rotate(360deg); } uses translate while the from state uses translateX(-50%). This breaks the consistent rotation center and can change the visual starting/ending position of the hands, which should remain at the top according to the task. Make both from and to use the same translate (e.g. translateX(-50%)) so the hand rotates smoothly around a fixed point.

Comment thread src/styles/components/stopwatch.scss Outdated
Comment on lines +40 to +46
&--speed-up {
.stopwatch__hand--seconds {
animation-duration: 10s;
}
.stopwatch__hand--minutes {
animation-duration: 600s;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inside the modifier &--speed-up you directly nest .stopwatch__hand--seconds and .stopwatch__hand--minutes. This can be considered a BEM “typical mistake”, because modifiers should modify the block/element via consistent selector composition (e.g. .stopwatch--speed-up .stopwatch__hand--seconds) instead of redefining elements via nested full class selectors. Refactor this section to follow BEM best practices as required by checklist item #3.

Comment thread src/styles/index.scss
@@ -1,3 +1,9 @@
@import './components/stopwatch';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The stopwatch container is correctly sized and styled, but it is not actually vertically centered in all cases because body uses min-height: 100vh instead of height: 100vh. This violates the requirement to "place it in the center of the page (vertically and horizontally)" and checklist item #2: "Stopwatch is centered and has the correct arrows size". Consider making the page container (e.g. body) a full-height flexbox with height: 100vh so centering is guaranteed.

Comment thread src/styles/index.scss Outdated

body {
margin: 0;
min-height: 100vh;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using min-height: 100vh instead of height: 100vh can prevent strict vertical centering of the stopwatch because the flex container may grow beyond the viewport. Since the requirement says the stopwatch must be centered vertically and horizontally, update this to height: 100vh to guarantee centering across states.

@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

Your solution is approved because it fully meets the functional and checklist requirements for the stopwatch task. The stopwatch is correctly centered using a full-height flex body, and the main block has the required 80vmin x 80vmin size with a 1vmin dotted, circular black border. Both hands have the specified dimensions, colors, and positions, and they start from the top (12 o’clock) thanks to bottom: 50%, left: 50%, and transform-origin: bottom. The seconds hand rotates smoothly with animation: tick 60s linear infinite, while the minutes hand uses animation: tick 3600s steps(60) infinite, matching the timing and steps(60) requirement. The @keyframes tick definition correctly uses from/to with consistent translateX(-50%) rotate(...), preventing any rotation-center shift. The stopwatch--speed-up BEM modifier properly overrides animation durations to 10s for seconds and 600s for minutes without breaking base behavior, and your BEM structure avoids typical mistakes. Nicely done implementing all the details precisely and keeping the code clean.


✨ 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