Skip to content

solution: layout stop watch#4179

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

solution: layout stop watch#4179
AlexanderOkhotnichenko wants to merge 2 commits into
mate-academy:masterfrom
AlexanderOkhotnichenko:develop

Conversation

@AlexanderOkhotnichenko

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 two core requirements of the task are not yet correctly implemented, and they will cause the tests to fail. First, the task explicitly requires an HTML element with class stopwatch and a speed-up modifier on that same block; currently your markup and SCSS use watch / watch--speed-up, so tests looking for .stopwatch and .stopwatch--speed-up will not find the correct element (e.g. in src/index.html and src/styles/blocks/watch.scss). You should rename the block to stopwatch everywhere (HTML and SCSS) and ensure the modifier is stopwatch--speed-up, applied like class="stopwatch stopwatch--speed-up" in the HTML. Second, the speed-up modifier does not actually change the animation durations: inside &--speed-up you only define a local $watch-duration: 10s that is never used, so the seconds and minutes hands still animate with the base 60s / 60min durations. To fix this, override the animation properties for .stopwatch__second-hand and .stopwatch__minute-hand inside &--speed-up so they use 10s and 10min respectively (keeping linear for seconds and steps(60) for minutes). As a minor improvement, you can remove the unused $watch-duration variable once you directly control the animations via the modifier. Overall, your visual implementation and base animations look good, and once you align the class naming and implement the real speed-up behavior, your solution should fully meet the task requirements.


✨ 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 Outdated
<body>
<h1>Stop watch</h1>
<main class="main">
<div class="watch watch--speed-up">

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 task requires: Create an HTML element with the class stopwatch (this element is required for the tests). Here the block is named watch instead of stopwatch, so tests looking for .stopwatch will fail. Consider renaming the block class in HTML and the corresponding SCSS to stopwatch and stopwatch--speed-up.

Comment thread src/index.html
Comment on lines +10 to 11
minimum-scale=1.0"
/>

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 speed-up modifier is supposed to change animation durations (seconds hand to 10s, minutes hand to 10min), but here it only sets a local $watch-duration: 10s variable that is never used in the animation properties. This violates the requirement that With the speed-up modifier, it will take only 10s/10min for the hands to make a full circle. You should override the animation values for &__second-hand and &__minute-hand inside &--speed-up.

Comment thread src/styles/blocks/watch.scss Outdated
border-radius: 50%;
z-index: 10;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You are required to "Create an HTML element with the class stopwatch" and "Add the modifier to the stopwatch for tests". Using class="watch watch--speed-up" violates that requirement because there is no stopwatch block or stopwatch--speed-up modifier in the HTML.

Comment thread src/styles/blocks/watch.scss Outdated
Comment on lines +10 to +11
&--speed-up {
$watch-duration: 10s;

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 &--speed-up modifier currently only sets a local $watch-duration: 10s; variable that is never used, so it does not change the seconds or minutes hand animation durations to 10s/10min as required. The modifier must override the values used by $watch-speed / $watch-speed-minute (or directly override the animation declarations) so the hands actually speed up.

Comment thread src/styles/main.scss
Comment on lines +10 to +11
}

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 &--speed-up modifier is supposed to change the animation duration to 10s / 10min, but here it only declares a local $watch-duration: 10s; variable that is never used. This violates the requirement: With the speed-up modifier, it will take only 10s for the seconds hand ... and 10min for the minutes hand ... (change animation duration).

Comment on lines +10 to +11
transform-origin: bottom;
}

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 functional requirement: With the speed-up modifier, it will take only 10s for the seconds hand ... and 10min for the minutes hand .... Here you only define a local $watch-duration: 10s; inside the modifier, but the animations use $watch-speed and $watch-speed-minute defined in variables, so the modifier does not actually change the animation timing. Consider overriding the relevant variables or using different animation declarations within &--speed-up so the durations become 10s and 10min when the modifier is applied.

@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. You correctly use a .stopwatch block with the stopwatch--speed-up modifier, BEM naming for internal elements, and center the stopwatch via flexbox with the required 80vmin size and 1vmin dotted border. Both hands start from the top using bottom: 50% and transform-origin: bottom, and their lengths, widths, and colors match the specified values, including the 5vmin center circle with the correct color. Animations rely only on CSS keyframes with from/to and transform: rotate(...), the seconds hand rotates linearly in 60s by default, and the minutes hand uses steps(60) over 60 minutes, satisfying [CHECKLIST ITEM #1] and [CHECKLIST ITEM #2]. The stopwatch--speed-up modifier properly adjusts the custom property so the seconds hand completes a full circle in 10s and the minutes hand in 10 minutes while preserving timing functions, addressing [CHECKLIST ITEM #3]. I don’t see any critical blockers or violations of the task requirements; overall, this is a clean, correct implementation. Nicely done—this is a solid example of using CSS animations and BEM structure effectively.


✨ 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