Skip to content

resolved#4206

Open
kennyprog wants to merge 2 commits intomate-academy:masterfrom
kennyprog:develop
Open

resolved#4206
kennyprog wants to merge 2 commits intomate-academy:masterfrom
kennyprog:develop

Conversation

@kennyprog
Copy link
Copy Markdown

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Changes are needed before this can be considered complete, mainly because a required Sum usage is still incorrect and the checklist style rule is not followed.

Critical issues to fix:

  1. In src/App.jsx, the instance that should represent “nothing and 5” is currently written as something like <Sum a={5} />; per the task, this must be <Sum b={5} /> so that the component renders Sum of 0 and 5 is 5 and the corresponding Cypress test can pass.
  2. Both in src/App.jsx and in src/components/Sum/Sum.jsx, the <p> content uses separate JSX expressions (Sum of {a} and {b} is {a + b}), which violates [CHECKLIST ITEM #2] requiring string interpolation inside tag content; update these to use a single template literal inside braces, for example: <p>{Sum of ${a} and ${b} is ${a + b}}</p>.

Once you correct the b={5} usage and refactor the paragraph content to use string interpolation, your implementation will satisfy both the functional requirements and the explicit checklist rules. You already have the named export, default props, and most required Sum combinations implemented correctly, so you’re very close—these small but important fixes will bring everything in line with the task expectations.


✨ 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/App.jsx Outdated
<Sum a={-5} b={5}/>
<Sum a={10} b={0}/>
<Sum a={10} />
<Sum a={5} />
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 task requirement: The App must contain a Sum with just b = 5 (description: '- just b = 5). On this line you have <Sum a={5} /> which passes a instead of b. Replace it with <Sum b={5} /> so the required case is present.

Comment thread src/App.jsx
@@ -1,14 +1,14 @@
import React from 'react';
import './App.scss';
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 checklist item #2: '2. [CODE STYLE] - Use string interpolation inside tag content'. The current code uses separate JSX expressions (Sum of {a} and {b} is {a+b}). Use a template literal inside braces for the paragraph content, for example: <p>{Sum of ${a} and ${b} is ${a + b}}</p>.

Comment thread src/components/Sum/Sum.jsx Outdated
@@ -1 +1,3 @@
// export const Sum = () => ();
export const Sum = ({a = 0, b = 0}) => (
<p>Sum of {a} and {b} is {a+b}</p>
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 doesn't follow the checklist item: 'Use string interpolation inside tag content'. Your

currently uses separate expressions and text (Sum of {a} and {b} is {a+b}) which matches the BAD EXAMPLE. Consider using a template string like {Sum of ${a} and ${b} is ${a + b}} inside the

.

Copy link
Copy Markdown

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

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, while your Sum component itself is implemented correctly (named export, default values of 0, and the required <p>{Sum of ${a} and ${b} is ${a + b}}</p>), the page-level behavior that the tests cover is still broken.

The Cypress tests show that after changing only the second value to 5, the page still displays Sum of 10 and 0 is 10 instead of Sum of 0 and 5 is 5, and after clearing both inputs it still shows Sum of 0 and 5 is 5 instead of Sum of 0 and 0 is 0. This means the inputs in src/App.jsx (the ones used in the tests, e.g., data-cy="firstInput" and data-cy="secondInput") are not correctly updating the state when they are cleared or when only one of them is changed, so the UI does not treat “missing” values as 0 the way the component API is supposed to. You should make both inputs fully controlled by React state and, in their onChange handlers, convert '' to 0 so that clearing an input actually resets the corresponding state (a or b) to 0, e.g. using value === '' ? 0 : Number(value). After that, ensure the sum text on the page is driven directly from those state values with the exact wording Sum of ${a} and ${b} is ${a + b} so it matches the test expectations.

Once you fix the state handling in App.jsx and the Cypress tests for “nothing and 5” and “no params” pass, your solution will fully satisfy both the component and page requirements. You’ve already done a good job with the core Sum implementation and the required static usages; focus now on making the interactive page correctly reflect the same defaulting behavior.


✨ 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

Copy link
Copy Markdown

@2pasha 2pasha left a comment

Choose a reason for hiding this comment

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

lgtm 🔥

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