Skip to content

Add task solution#1789

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

Add task solution#1789
pzwolenik wants to merge 2 commits intomate-academy:masterfrom
pzwolenik:develop

Conversation

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

Review Summary

Great work on this task! Your application is fully functional and meets all the core requirements. The logic for selecting, displaying, and clearing goods works correctly.

I have one important recommendation for improvement regarding your event handlers. In src/App.tsx, using an inline arrow function for the onClick event creates a new function on every render, which can impact performance. This goes against a best practice noted in the checklist [CHECKLIST ITEM #2]. A better approach is to create a single event handler method that determines which item was clicked, for example, by using a data- attribute.

Since the functionality is correct, your solution is approved. Keep this performance best practice in mind for future projects!


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

Footnotes

  1. Rate AI review example

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

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: 'Create separate methods instead of creating inline event handlers'. Using an inline arrow function for onClick creates a new function on every render, which can impact performance.

Consider creating a single event handler method on the class that can determine which 'good' was clicked, for example, by using a data- attribute on the button.

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