Skip to content

add task solution#3837

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

add task solution#3837
goffa-exe wants to merge 2 commits into
mate-academy:masterfrom
goffa-exe:develop

Conversation

@goffa-exe
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

Decision: REQUEST_CHANGES

Your Sum component is generally well-structured and uses defaults correctly—nice job there. However, there are critical blockers in App.jsx that violate required checklist items and prevent the task from being considered complete.

Critical issues to fix

  1. src/App.jsx (all five instances)

    • Issue: All Sum usages pass both a and b as strings (e.g., a="2" b="3"). This violates checklist item #1 and can produce incorrect behavior. Props must be numbers, not strings.
    • Why: The requirement explicitly states to pass numeric values and to render exactly five instances with specific prop combinations, including cases with only a, only b, and no props.
    • How to fix:
      • Use numeric props: a={2}, not a="2".
      • Render exactly these five instances on a single line each:
        • a={2} b={3}
        • a={-5} b={5}
        • a={10} (no b)
        • b={5} (no a)
        • no props: <Sum />
    • Examples:
      • Use <Sum a={10} /> instead of <Sum a="10" b="..." />.
      • Use <Sum b={5} /> instead of <Sum a="..." b="5" />.
      • Include <Sum /> with no props.
  2. src/components/Sum/Sum.jsx

    • Issue: The paragraph builds the sentence with multiple JSX expressions instead of one template literal. This violates checklist item #2.
    • Why: The checklist requires a single string interpolation expression inside the paragraph content for clarity and consistency.
    • How to fix: Use one template literal in the JSX paragraph, for example: Sum of ${a} and ${b} is ${Number(a) + Number(b)} inside a single {}.

Minor notes

  • Keep each Sum usage on one line when there are fewer than three short attributes (you’re already close here; just correct the values/props as above).

You’re close—fixing the prop passing in App.jsx and using a single interpolation in Sum.jsx will fully align your solution with the requirements. Keep it up!


✨ 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
<p>Sum of 0 and 0 is 0</p>
{/* Replace paragraphs with Sum componets */}
{/* And remove commented lines :) */}
<Sum a="2" b="3" />
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 #1: 'In App, render exactly five Sum component instances with the following props: ... a = 10 only (no b prop), b = 5 only (no a prop), no props at all.' All Sum components here have both a and b props, and all values are passed as strings instead of numbers. You must:

  • Pass numeric values (e.g., a={2}, not a="2")
  • Provide instances with only a, only b, and no props as required.

Comment thread src/App.jsx Outdated
{/* Replace paragraphs with Sum componets */}
{/* And remove commented lines :) */}
<Sum a="2" b="3" />
<Sum a="-5" 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.

This violates checklist item #1: 'In App, render exactly five Sum component instances with the following props: ... a = 10 only (no b prop), b = 5 only (no a prop), no props at all.' All Sum components here have both a and b props, and all values are passed as strings instead of numbers. You must:

  • Pass numeric values (e.g., a={-5}, not a="-5")
  • Provide instances with only a, only b, and no props as required.

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.

This violates checklist item #1: 'In App, render exactly five Sum component instances with the following props: ... a = 10 only (no b prop), b = 5 only (no a prop), no props at all.' This instance should be (no b prop), but both a and b are provided as strings.

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.

This violates checklist item #1: 'In App, render exactly five Sum component instances with the following props: ... a = 10 only (no b prop), b = 5 only (no a prop), no props at all.' This instance should be (no a prop), but both a and b are provided as strings.

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

This violates checklist item #1: 'In App, render exactly five Sum component instances with the following props: ... a = 10 only (no b prop), b = 5 only (no a prop), no props at all.' This instance should be (no props at all), but both a and b are provided as strings.

Comment thread src/components/Sum/Sum.jsx Outdated
// export const Sum = () => ();
export const Sum = ({ a = 0, b = 0 }) => (
<p>
Sum of {a} and {b} is {Number(a) + Number(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 the paragraph’s JSX content to compose the sentence in one expression, e.g., {Sum of ${a} and ${b} is ${a + b}}, rather than concatenating multiple JSX fragments.'

You should use a single string interpolation expression inside the paragraph, such as:

<p>{`Sum of ${a} and ${b} is ${Number(a) + Number(b)}`}</p>

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

Decision: APPROVE

Your implementation fully meets the requirements:

  • The Sum component is a named export, accepts a and b with default values of 0, and renders the exact text format inside a single template literal within a <p> tag.
  • App.jsx correctly renders all five required Sum usages on single lines, passing numeric props as numbers (e.g., a={2}), and covers both, only a, only b, and neither cases.

No critical blockers or functional issues were found. Nice job keeping the code concise and readable. If you want a tiny polish, the Number(a)/Number(b) conversions in Sum are unnecessary given numeric defaults, but they don’t cause any problems. Great work!


✨ 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