Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing all the required features! The application works as expected. However, there are a few critical issues related to React best practices and our project guidelines that need to be addressed before I can approve this.
- Key Prop: You are using the array index as a
keywhen rendering the list of goods. This is a direct violation of our checklist [CHECKLIST ITEM #4] and can cause unpredictable UI behavior. Please use a unique and stable value from your data, such as thegoodname itself:key={good}. - Event Handlers: You are creating new arrow functions for
onClickhandlers. According to our guidelines [CHECKLIST ITEM #3], you should pass the method reference directly when no arguments are needed, for example:onClick={this.clearSelectedGood}. - TypeScript Type: The component's type is specified as
React.Component<State>, which is incorrect. Since the component doesn't receive props, it should beReact.Component<{}, State>.
Please fix these issues. You're very close to a solid solution!
✨ 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
src/App.tsx
Outdated
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} | ||
| <button data-cy="ClearButton" type="button" className="delete ml-3" /> | ||
| </h1> | ||
| export class App extends React.Component<State> { |
There was a problem hiding this comment.
The type for React.Component is not correctly specified. It should be React.Component<Props, State>. Since this component doesn't receive any props, you should use an empty object {} for the Props type: React.Component<{}, State>.
| deleteGood = () => { | ||
| this.setState({ | ||
| selectedGood: '', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This method has the same implementation as clearSelectedGood. To avoid code duplication and follow the DRY (Don't Repeat Yourself) principle, you can remove one of them and use the other in both places.
src/App.tsx
Outdated
| </td> | ||
| </tr> | ||
| className="delete ml-3" | ||
| onClick={() => this.clearSelectedGood()} |
There was a problem hiding this comment.
This violates checklist item #3: "If your method get event as argument or don't get any argument, you don't need to create new arrow function. Just pass your method." Since clearSelectedGood doesn't require any arguments, you should pass the method reference directly: onClick={this.clearSelectedGood}.
src/App.tsx
Outdated
| {goods.map((good, index) => { | ||
| return ( | ||
| <tr | ||
| key={index} |
There was a problem hiding this comment.
This violates checklist item #4: "NEVER EVER EVER use array index as a key prop". Using the array index as a key can cause unpredictable behavior if the order of items changes. Since the good names are unique, it's better to use good as the key: key={good}.
src/App.tsx
Outdated
| data-cy="RemoveButton" | ||
| type="button" | ||
| className="button is-info" | ||
| onClick={() => this.deleteGood()} |
There was a problem hiding this comment.
This violates checklist item #3: "If your method get event as argument or don't get any argument, you don't need to create new arrow function. Just pass your method." Similar to the ClearButton, you can pass the method reference directly here since deleteGood takes no arguments: onClick={this.deleteGood}.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this revision! You have successfully addressed all the critical feedback, and the application now fully meets all functional requirements. I am approving your solution.
As a suggestion for future improvement, I noticed a small area of code duplication in src/App.tsx. The deleteGood method is identical to clearSelectedGood. You could remove one and reuse the other for both the ClearButton and RemoveButton handlers to make the code cleaner. This is just a minor point for maintainability, not a required change.
Great job overall!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| deleteGood = () => { | ||
| this.setState({ | ||
| selectedGood: '', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This method has the exact same functionality as clearSelectedGood. You can remove this one and reuse clearSelectedGood for the RemoveButton's onClick handler to avoid code duplication.
3 participants
No description provided.