Skip to content

Solution#2032

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

Solution#2032
JPodgrodzki wants to merge 2 commits into
mate-academy:masterfrom
JPodgrodzki:develop

Conversation

@JPodgrodzki

Copy link
Copy Markdown

No description provided.

@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

There are several critical blockers that must be addressed before this submission can be approved:

  1. Incorrect Property Names in Data and Types

    • Throughout your code (in App.tsx, PreparedPost.ts, and PostInfo.tsx), you use foundUser and foundComments instead of the required user and comments properties. This directly violates the requirement: "The data for posts must be prepared by adding a user and comments to each post." It will break component expectations and cause test failures.
    • How to fix: Rename all instances of foundUser to user and foundComments to comments in your data preparation, types, and component props.
  2. Improper Use of key Prop in Info Components

    • In both CommentInfo.tsx and PostInfo.tsx, you set the key prop in the child (Info) component. The checklist explicitly states: "The key prop must not be specified in the child component." This can cause rendering bugs and will break tests.
    • How to fix: Remove the key prop from the root element of your Info components. Ensure key is only set in the parent component when mapping over lists.
  3. Styles Not Fully Migrated from App.scss

    • App.tsx still imports App.scss, which violates the requirement to move styles to component-specific files (e.g., PostList.scss).
    • How to fix: Move all relevant styles from App.scss to the appropriate component .scss files and remove the import from App.tsx.
  4. Prop Naming in List Components

    • The prop passed to PostList is named preparedPosts, but the requirements and tests expect posts.
    • How to fix: Rename the prop to posts in both the parent and the PostList component.
  5. String Interpolation in JSX

    • In PostInfo.tsx, you concatenate strings in JSX instead of using string interpolation, which is required by the checklist.
    • How to fix: Use template literals or direct interpolation inside your JSX tags.

Next Steps:

  • Address all the above issues, making sure to update property names, remove improper key usage, migrate styles, and fix prop naming and string interpolation.
  • Double-check that all requirements and checklist items are satisfied, as these issues are likely to cause test failures.

You're on the right track with your component structure and type usage—just make these critical adjustments and you'll be set! If you have questions about any of the requirements, feel free to ask for clarification.


✨ 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.tsx Outdated

return {
...post,
foundUser,

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: 'The data for posts must be prepared by adding a user and comments to each post.' You are adding foundUser and foundComments instead of user and comments.

Comment thread src/App.tsx Outdated
return {
...post,
foundUser,
foundComments,

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: 'The data for posts must be prepared by adding a user and comments to each post.' You are adding foundUser and foundComments instead of user and comments.

Comment thread src/App.tsx Outdated
@@ -1,106 +1,27 @@
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 the checklist item: 'Styles from App.scss must be moved to separate files corresponding to components (e.g., PostList.scss).' You are still importing App.scss here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

What about:
iframe {
display: none;
}

.App__title {
text-align: center;
}
in App.scss? its mandatory to maintain the look, but it can't be moved to any other component

Comment thread src/App.tsx Outdated
</div>
</div>
</div>
<PostList preparedPosts={preparedPosts} />

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 checklist item: 'List components must take an array of corresponding objects and render one Info component per object.' The prop name preparedPosts may not match what PostList expects (likely posts). Double-check prop naming for consistency with requirements and tests.


export const CommentInfo: React.FC<Props> = ({ comment }) => {
return (
<div className="CommentInfo" key={comment.id}>

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 checklist item: 'The key prop must not be specified in the child component.' The key should be set in the parent component when mapping, not here in the Info component.

<h3 className="PostInfo__title">{post.title}</h3>

<p>
{' Posted by '}

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 checklist item: 'String interpolation must be used inside tag content instead of concatenating with {} and spaces.' You should use string interpolation directly in the tag content for 'Posted by'.

Comment thread src/components/PostList/PostList.tsx Outdated

export const PostList: React.FC = () => <>Put the list here</>;
type Props = {
preparedPosts: PreparedPost[];

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 checklist item: 'List components must take an array of corresponding objects and render one Info component per object.' The prop should likely be named posts instead of preparedPosts to match requirements and tests.

Comment thread src/components/PostList/PostList.tsx Outdated
preparedPosts: PreparedPost[];
};

export const PostList: React.FC<Props> = ({ preparedPosts }) => (

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 checklist item: 'List components must take an array of corresponding objects and render one Info component per object.' The prop should likely be named posts instead of preparedPosts to match requirements and tests.

Comment thread src/types/PreparedPost.ts Outdated
import { Comment } from './Comment';

export type PreparedPost = Post & {
foundUser: User | null;

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: 'The data for posts must be prepared by adding a user and comments to each post.' The properties should be named user and comments, not foundUser and foundComments.

Comment thread src/types/PreparedPost.ts Outdated

export type PreparedPost = Post & {
foundUser: User | null;
foundComments: 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.

This violates the requirement: 'The data for posts must be prepared by adding a user and comments to each post.' The properties should be named user and comments, not foundUser and foundComments.

@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

All core requirements and checklist items are fully met. Your implementation correctly prepares and renders the posts with their associated users and comments, uses the required component and styling structure, and follows all technical and code style constraints. There are no critical or blocking issues—everything is clean, modular, and testable. Great job addressing all previous feedback and demonstrating strong attention to detail! Keep up the excellent 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