Skip to content

fix: resolve stopwatch pixel alignment and subpixel rounding issue#4160

Open
ageev2506 wants to merge 3 commits into
mate-academy:masterfrom
ageev2506:develop
Open

fix: resolve stopwatch pixel alignment and subpixel rounding issue#4160
ageev2506 wants to merge 3 commits into
mate-academy:masterfrom
ageev2506:develop

Conversation

@ageev2506

Copy link
Copy Markdown

❗️ Copy this Checklist to the Pull Request description after links, and put - [x] before each point after you checked it.

  • [x ] Keyframes implemented using from/to + transform with rotate property
  • [x ] Stopwatch is centered and has the correct arrows size
  • [x ] All Typical Mistakes from the BEM lesson theory are checked.
  • [ x] 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 several core task requirements and checklist items are not yet satisfied, which will also cause tests to fail. The main stopwatch element must have class stopwatch with a BEM modifier like stopwatch stopwatch--speed-up, but currently the block is named clock and there is no speed-up modifier in HTML or CSS. The stopwatch also needs to be fully centered vertically and horizontally (e.g. using flexbox on body), while right now it is only centered horizontally. Key visual specs are incorrect: border must be 1vmin dotted #000; the minute hand must be 20vmin x 3vmin and blue #0700ff; the second hand must be 38vmin x 1.5vmin and green #2c8000; and the center circle must be 5vmin in diameter and orange #f6a603. Animation rules are also off: the seconds hand must rotate in 60s linear, the minutes hand must use 60min (3600s) with steps(60, ...), keyframes must use from/to with transform: rotate(...), and the speed-up modifier should change these to 10s and 10min respectively. Please rename the block to stopwatch, add the stopwatch--speed-up modifier, fix centering, sizes, colors, and animation timings to match the specification, and then your solution will be very close to complete; the basic structure and use of CSS animations are a good starting point to build on.


✨ 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
<h1>Stop watch</h1>
<h1 class="title">Stop watch</h1>

<div class="clock">

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 requirement Create an HTML element with the class stopwatch (this element is required for the tests) and the BEM modifier requirement. You need a block named stopwatch (e.g. <div class="stopwatch stopwatch--speed-up">…</div>) instead of clock, and a speed-up modifier class for the accelerated mode.

Comment thread src/index.html
href="./styles/index.scss"
/>
</head>

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 border should be 1vmin dotted #000 as per the description. Currently it is 10px dotted rgb(160, 0, 0), which does not match the specified size or color.

Comment thread src/styles/index.scss
background: rgb(0, 0, 0);
position: relative;
margin: 0 auto;

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 main stopwatch element is required to have class stopwatch according to the description: tests expect this block, and BEM modifier speed-up must be applied to it. Using .clock here and having no .stopwatch element will cause tests and checklist item about the stopwatch block and modifier to fail.

Comment thread src/styles/index.scss
background: rgb(0, 0, 0);
position: relative;
margin: 0 auto;

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 also need to add the speed-up BEM modifier class (e.g. stopwatch stopwatch--speed-up) to the stopwatch block in HTML for tests, per the description; it’s currently missing entirely.

Comment thread src/styles/index.scss
width: 80vmin;
border-radius: 80vmin;
border: 10px dotted rgb(160, 0, 0);
background: rgb(0, 0, 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 stopwatch must be centered both vertically and horizontally. At the moment, the body isn’t using flexbox or another method to center .clock vertically, so this violates the requirement “place it in the center of the page (vertically and horizontally)” and checklist item "Stopwatch is centered".

Comment thread src/styles/index.scss
height: 80vmin;
width: 80vmin;
border-radius: 80vmin;
border: 10px dotted rgb(160, 0, 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 stopwatch’s border must be 1vmin dotted #000 according to the description; this 10px dotted rgb(160, 0, 0) border violates that requirement.

Comment thread src/styles/index.scss
Comment on lines +24 to +27
&__hours {
height: 20vmin;
width: 2vmin;
background: red;

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 is required to be 20vmin long, 3vmin wide, and blue #0700ff. Here it’s 20vmin high, 2vmin wide, red, and named as __hours, which doesn’t match the spec for the minute hand’s size and color.

Comment thread src/styles/index.scss
Comment on lines +35 to +43
&__minutes {
height: 40vmin;
width: 1vmin;
background: red;
position: absolute;
top: 1vmin;
left: 39.5vmin;
transform-origin: 0.5vmin 39vmin;
animation: 15s linear infinite rotate360;

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 seconds hand must be 38vmin long, 1.5vmin wide, green #2c8000, and use 60s duration with even speed (linear). This element is 40vmin by 1vmin, red, and uses 15s duration, so it doesn’t meet the seconds hand requirements.

Comment thread src/styles/index.scss
Comment on lines +35 to +43
&__minutes {
height: 40vmin;
width: 1vmin;
background: red;
position: absolute;
top: 1vmin;
left: 39.5vmin;
transform-origin: 0.5vmin 39vmin;
animation: 15s linear infinite rotate360;

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 should use a 60min (3600s) animation with steps(60), not 15s linear. This conflicts with the requirement: "For minutes hand use steps animation with 60 steps... 60min to make a full circle."

Comment thread src/styles/index.scss
Comment on lines +45 to +50
&__dot {
height: 4vmin;
width: 4vmin;
border-radius: 5vmin;
border: 3px solid red;
background: white;

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 center circle should have 5vmin diameter and orange #f6a603. Here it’s 4vmin with a red border and white fill, which doesn’t match the specification.

@Anton-Kuchmasov Anton-Kuchmasov 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.

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.

3 participants