Solution#1778
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
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.
| data-cy="AddButton" | ||
| type="button" | ||
| className="button" | ||
| onClick={() => this.selectGood(good)} |
There was a problem hiding this comment.
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.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
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! ✨
| data-cy="AddButton" | ||
| type="button" | ||
| className="button" | ||
| onClick={() => this.choosedGood(good)} |
There was a problem hiding this comment.
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)
https://webesperer.github.io/react_goods-selector/