fix: first#4177
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a few checklist-required behaviors are still missing or incorrect. First, both @keyframes move and @keyframes move2 are defined with 0%/100% instead of from/to, which violates the explicit animation requirement [CHECKLIST ITEM #1]; please switch those keyframes to use from and to with transform: rotate(...). Second, the speed-up BEM modifier on the stopwatch block is not implemented: the root <div> in src/index.html only has class="stopwatch", and there is no corresponding CSS for .stopwatch--speed-up (or .stopwatch_speed-up) to change the seconds hand to 10s per rotation and the minutes hand to 10min per rotation as required. You should add the modifier class in HTML (e.g. class="stopwatch stopwatch--speed-up") and create SCSS rules that override animation-duration for both hands when that modifier is present [CHECKLIST ITEM #2]. Third, the center element is named stopwatch-center, which is not valid BEM for the stopwatch block; rename it consistently in HTML and SCSS to something like stopwatch__center to avoid the “block-like element” mistake [CHECKLIST ITEM #3]. Once you fix the keyframes syntax, add and style the speed-up modifier, and correct the center element’s BEM name, your solution will align with all core requirements and checklist items; the rest of the visuals and animations look solid, so 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
| <body> | ||
| <h1>Stop watch</h1> | ||
| <div class="stopwatch"> | ||
| <div class="stopwatch-center"></div> | ||
| <div class="stopwatch__minutes-hand"></div> | ||
| <div class="stopwatch__seconds-hand"></div> | ||
| </div> | ||
| </body> |
There was a problem hiding this comment.
Same keyframes issue here: using 0%/100% instead of from/to violates the keyframes checklist requirement. Consider aligning this with the required pattern.
| <div class="stopwatch"> | ||
| <div class="stopwatch-center"></div> | ||
| <div class="stopwatch__minutes-hand"></div> | ||
| <div class="stopwatch__seconds-hand"></div> | ||
| </div> |
There was a problem hiding this comment.
The root stopwatch element currently has only class="stopwatch", but the task requires adding the speed-up modifier for tests (e.g. class="stopwatch stopwatch--speed-up"). Make sure to add the modifier class so that tests can detect the sped-up animation.
|
|
||
| @keyframes move2 { | ||
| 0% { | ||
| transform: rotate(0deg); |
There was a problem hiding this comment.
You haven't added the required BEM modifier speed-up on the stopwatch block (e.g. class="stopwatch stopwatch_speed-up"), which violates the description requirement and checklist item #2 indirectly, since tests depend on this modifier for the accelerated animation.
| @keyframes move2 { | ||
| 0% { | ||
| transform: rotate(0deg); | ||
| } |
There was a problem hiding this comment.
The center element is named stopwatch-center, which is not a valid BEM element for the stopwatch block. This conflicts with checklist item #3 about avoiding typical BEM mistakes; consider renaming to stopwatch__center.
| @keyframes move { | ||
| 0% { | ||
| transform: rotate(0deg); | ||
| } | ||
| 100% { | ||
| transform: rotate(360deg); | ||
| } | ||
| } |
There was a problem hiding this comment.
Checklist item #1 requires "Keyframes implemented using from/to + transform with rotate property", but here you are using 0% and 100%. Functionally it works, but it violates that specific checklist requirement; switch to from/to.
| @keyframes move2 { | ||
| 0% { | ||
| transform: rotate(0deg); | ||
| } | ||
| 100% { | ||
| transform: rotate(360deg); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as for @keyframes move, this keyframes definition uses 0%/100% instead of from/to, which violates checklist item #1 about keyframes syntax.
| .stopwatch__minutes-hand { | ||
| position: absolute; | ||
| height: 20vmin; | ||
| width: 3vmin; | ||
| background-color: #0700ff; | ||
| top: 50%; | ||
| left: 50%; | ||
| transform-origin: bottom center; | ||
| margin-left: -1.5vmin; | ||
| margin-top: -20vmin; | ||
|
|
||
| animation-name: move; | ||
| animation-duration: 3600s; | ||
| animation-timing-function: steps(60, end); | ||
| animation-iteration-count: infinite; | ||
| animation-delay: 0s; | ||
| z-index: 1; | ||
| } | ||
|
|
||
| .stopwatch__seconds-hand { | ||
| position: absolute; | ||
| height: 38vmin; | ||
| width: 1.5vmin; | ||
| background-color: #2c8000; | ||
| top: 50%; | ||
| left: 50%; | ||
| transform-origin: bottom center; | ||
| margin-left: -0.75vmin; | ||
| margin-top: -38vmin; | ||
|
|
||
| animation-name: move2; | ||
| animation-duration: 60s; | ||
| animation-timing-function: linear; | ||
| animation-iteration-count: infinite; | ||
| z-index: 2; | ||
| } |
There was a problem hiding this comment.
There is no CSS for a stopwatch_speed-up modifier that changes the animation durations to 10s (seconds hand) and 10min (minutes hand). This is required by the description and for tests; you need to add modifier styles overriding animation-duration for both hands when the modifier is present.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
brespect
left a comment
brespect
left a comment
There was a problem hiding this comment.
Good progress, but you need to pass all tests and share your working DEMO LINK to the PR description before requesting the review
3 participants
No description provided.