Skip to content

Solution#4259

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

Solution#4259
Rostyslav452 wants to merge 3 commits into
mate-academy:masterfrom
Rostyslav452:develop

Conversation

@Rostyslav452
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

This requires changes due to several critical issues blocking task completion.

Critical Issues:

  1. Export mismatch (build error): App.jsx uses import Sum from './components/Sum/Sum' but Sum.jsx only has a named export. Either add export default Sum to Sum.jsx or change the import in App.jsx to `import { Sum } from './components/Sum/Sum'.

  2. Wrong number of Sum instances: The requirement specifies exactly 5 instances, but you have 6. You need to remove the extra instances that don't match the specification.

  3. Props passed incorrectly: The requirements specify testing default prop behavior, but your code passes explicit zeros instead of omitting props:

    • <Sum a={10} b={0} /> should be <Sum a={10} /> (omit b to test default)
    • <Sum a={0} b={5} /> should be <Sum b={5} /> (omit a to test default)
    • <Sum a={0} b={0} /> should be <Sum /> (omit both to test defaults)
  4. String concatenation instead of template literals: In Sum.jsx, checklist item #2 requires using template literals: `Sum of ${a} and ${b} is ${a + b}` instead of manual concatenation.

Once you fix the export issue, all 6 unit tests pass, which is great. Focus on fixing the export mismatch and adjusting the props to match the requirements exactly.


✨ 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

// export const Sum = () => ();
export const Sum = ({ a = 0, b = 0 }) => (
<p>
Sum of {a} and {b} is {a + b}
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: Use string interpolation inside tag content. Use template literals like {Sum of ${a} and ${b} is ${a + b}} instead of separate expressions.

Comment thread src/App.jsx Outdated
Comment on lines +1 to +3
import React from 'react';
import './App.scss';
import Sum from './components/Sum/Sum';
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: 'Use string interpolation inside tag content'. The GOOD example shows using template literals: {Sum of ${a} and ${b} is ${a + b}}. Change line 3 to use template literals instead of manual concatenation.

Comment thread src/App.jsx Outdated
{/* And remove commented lines :) */}
<Sum a={2} b={3} />
<Sum a={-5} b={5} />
<Sum a={10} b={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 requirement specifies 'just a = 10' (no b prop), but this passes b={0} explicitly. This should be <Sum a={10} /> to test the default prop behavior for 'b'.

Comment thread src/App.jsx Outdated
<Sum a={2} b={3} />
<Sum a={-5} b={5} />
<Sum a={10} b={0} />
<Sum a={0} b={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.

The requirement specifies 'just b = 5' (no a prop), but this passes a={0} explicitly. This should be <Sum b={5} /> to test the default prop behavior for 'a'.

Comment thread src/App.jsx Outdated
<Sum a={10} b={0} />
<Sum a={0} b={5} />
<Sum a={2} b={3} />
<Sum a={0} b={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 requirement specifies 'no params at all', but this passes a={0} b={0} explicitly. This should be <Sum /> with no props to test the default prop behavior.

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

The request meets the required criteria for approval.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/App.jsx
Comment on lines 1 to 3
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.

Checklist item #2 requires using template literals for string interpolation inside tag content. The GOOD example shows: {Sum of ${a} and ${b} is ${a + b}}. Currently using literal text with JSX expressions instead of a template literal.

Comment on lines +2 to +3
<p>
Sum of {a} and {b} is {a + b}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Checklist item #2 requires using template literals for string interpolation: wrap the template literal in curly braces like {Sum of ${a} and ${b} is ${a + b}} instead of inline JSX expressions.

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