Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react_dynamic-list-of-post solution #1180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

piyopiyo0
Copy link

Copy link

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

There are several critical issues that need to be addressed, including validation of the newComment object to prevent adding comments with empty data, and the use of any for type safety. Please address these issues to ensure the code meets the core requirements. Keep up the great work, and once these changes are made, your code will be in excellent shape! 😊

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

cypress/integration/page.spec.js Outdated Show resolved Hide resolved
cypress/integration/page.spec.js Outdated Show resolved Hide resolved
Comment on lines 50 to 51
Cypress.Commands.add('getByDataCy', selector => {
cy.get(`[data-cy="${selector}"]`);

Choose a reason for hiding this comment

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

The custom command 'getByDataCy' should return the result of 'cy.get' to ensure it can be chained properly.

src/App.tsx Show resolved Hide resolved
src/components/MainContent/MainContent.tsx Show resolved Hide resolved
src/utils/fetchClient.ts Outdated Show resolved Hide resolved
src/utils/fetchClient.ts Outdated Show resolved Hide resolved
src/utils/fetchClient.ts Outdated Show resolved Hide resolved
export const client = {
get: <T>(url: string) => request<T>(url),
post: <T>(url: string, data: any) => request<T>(url, 'POST', data),
patch: <T>(url: string, data: any) => request<T>(url, 'PATCH', data),

Choose a reason for hiding this comment

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

Using 'any' for the 'data' parameter is not recommended. Consider using a more specific type or a generic type parameter to ensure type safety.

src/utils/fetchFunctions.ts Show resolved Hide resolved
@piyopiyo0 piyopiyo0 changed the base branch from develop to master October 16, 2024 19:56
Copy link

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

Great job on your work! 🎉 While there are some minor improvements suggested, none of them critically impact the functionality or security of your code. Keep up the good work and consider the feedback for future enhancements!

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

const { activePost } = useContext(ActivePostContext);
const { posts } = useContext(PostsContext);
const { isError } = useContext(ErrorsContext);
const { isLoading } = useContext(LoaderContext);

Choose a reason for hiding this comment

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

The variable 'isLoading' is correctly named according to the checklist. Ensure that you do not use 'isLoad'.

Copy link
Author

Choose a reason for hiding this comment

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

I do :)

</div>
))}

{posts && <PostsList posts={posts} />}

Choose a reason for hiding this comment

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

Ensure that the 'PostsList' component has propTypes defined for the 'posts' prop, as required by the checklist.

className={classNames('button is-link', {
'is-light': activePost?.id !== post.id,
})}
onClick={() => handleOpenButtonClick()}

Choose a reason for hiding this comment

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

The event handler function handleOpenButtonClick should follow the naming conventions for event handler functions in React. Consider renaming it to handleOpenButtonClick to onOpenButtonClick or similar.

<tr className="has-background-link-light">
<th>#</th>
<th>Title</th>
{/* eslint-disable-next-line jsx-a11y/control-has-associated-label */}

Choose a reason for hiding this comment

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

Remove unused comments as per the checklist requirement.


<tbody>
{posts.map(post => (
<PostItem key={post.id} post={post} data-cy="Post" />

Choose a reason for hiding this comment

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

Avoid generating keys during render. Ensure that the key is stable and unique for each item in the list to prevent potential issues with component identity.

src/utils/Store.tsx Show resolved Hide resolved
src/utils/Store.tsx Show resolved Hide resolved
src/utils/Store.tsx Show resolved Hide resolved
src/utils/Store.tsx Show resolved Hide resolved
src/utils/fetchClient.ts Show resolved Hide resolved
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