Skip to content

Solution#1778

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

Solution#1778
webEsperer wants to merge 2 commits intomate-academy:masterfrom
webEsperer:develop

Conversation

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

Hi! 👋 The code is well-structured and aligns with the task requirements. However, I've noticed a few opportunities for improvement based on the checklist. Specifically, the onClick event handlers for both buttons in src/App.tsx should be bound directly to the methods (this.clearSelection and this.selectGood) instead of using inline arrow functions. This avoids creating new function instances on every render, improving performance. Could you please update those two event handlers? Thanks! 👍

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

🤖 If you have any comments about this review, please leave a message in the Add a comment field below and mention me with @mateacademy-ai-mentor tag. I will read your message when I perform the next review of your pull request.

Comment thread src/App.tsx Outdated
data-cy="AddButton"
type="button"
className="button"
onClick={() => this.selectGood(good)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

According to the checklist item 2:

[CODE KNOWLEDGE] - Create separate methods instead of creating inline event handlers (if we do so - we don't create new arrow function after component re-rendering)

It's better to define this.selectGood method separately and pass it to onClick.

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.

The code looks good and mostly follows the task requirements, but there's still a small issue with the AddButton in src/App.tsx. The onClick event handler onClick={() => this.choosedGood(good)} creates a new function instance on every render. To avoid this, you should bind this.choosedGood in the constructor or use arrow function syntax for the method definition. This is important for performance, especially in larger applications. I'm approving the PR to keep you moving forward, but please address this comment to improve your code. Keep up the great work!

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

Comment thread src/App.tsx
data-cy="AddButton"
type="button"
className="button"
onClick={() => this.choosedGood(good)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

According to the checklist, it is better to avoid inline event handlers. You can bind this.choosedGood in the constructor or use arrow function syntax for the method definition.

See checklist item:

2. [CODE KNOWLEDGE] - Create separate methods instead of creating inline event handlers (if we do so - we don't create new arrow function after component re-rendering)

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