Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on implementing the core functionality! The component works well visually. However, some changes are needed to align the code with the specific technical requirements of the task.
Here are the key issues to address:
- State Variable Naming: The state variable is named
marked, but the requirements explicitly state it must beselectedGood. - Incorrect Clear Value: When clearing the selection, the state is set to
null. The task requires using an empty string ('') for this purpose. - Inline Event Handlers: The
onClickhandlers are implemented as inline arrow functions. Please extract this logic into separate class methods as required by the project's guidelines [CHECKLIST ITEM #2].
Please address these points to fully meet the task's requirements. Keep up the great effort!
✨ 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
| </td> | ||
| export class App extends React.Component<Props, State> { | ||
| state: State = { | ||
| marked: 'Jam', |
There was a problem hiding this comment.
The requirements state that the selected item should be stored in a state variable named selectedGood. Please rename marked to selectedGood to match the requirements.
src/App.tsx
Outdated
| import './App.scss'; | ||
|
|
||
| type State = { | ||
| marked: string | null; |
There was a problem hiding this comment.
The task requires clearing the selection by setting the state to an empty string (''), but here null is used. Please use an empty string as specified.
src/App.tsx
Outdated
| data-cy="ClearButton" | ||
| type="button" | ||
| className="delete ml-3" | ||
| onClick={() => this.setState({ marked: null })} |
There was a problem hiding this comment.
This violates checklist item #2: "Create separate methods instead of creating inline event handlers". Please extract this logic into a class method and reference it here.
src/App.tsx
Outdated
| onClick={() => | ||
| good === this.state.marked | ||
| ? this.setState({ marked: null }) | ||
| : this.setState({ marked: `${good}` }) | ||
| } |
There was a problem hiding this comment.
This violates checklist item #2: "Create separate methods instead of creating inline event handlers". The logic for adding or removing a good should be moved into a separate class method.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the progress! You've correctly implemented most of the requirements, and the state management is now working perfectly. However, one change is needed before this can be approved. The onClick handler for the add/remove button in src/App.tsx contains logic directly inside an arrow function. This goes against the project requirement to create separate methods for event handlers [CHECKLIST ITEM #2]. Please extract this logic into a new class method that can then be called from the onClick handler. Once you've made this change, the solution will be ready for approval. Keep up the great work!
✨ 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
| onClick={() => | ||
| good === this.state.selectedGood | ||
| ? this.clearSelection() | ||
| : this.selectGood(good) | ||
| } |
There was a problem hiding this comment.
This event handler contains logic directly in the JSX. According to checklist item #2 ('Create separate methods instead of creating inline event handlers'), this logic should be extracted into a separate class method. You can create a new method that takes good as an argument and then call it from here.
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 PR! You've done a great job addressing the previous feedback by extracting the event handler logic into its own method. The application is now fully functional and meets all the requirements.
I am approving this solution so you can move forward, but please note there is one minor typo to fix. In src/App.tsx, the method handelClick should be renamed to handleClick. This is to comply with our style guide, which states that method names should start with a verb [CHECKLIST ITEM #1].
This is a very small correction in an otherwise fantastic submission. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| + | ||
| </button> | ||
| </td> | ||
| handelClick = (good: string) => { |
There was a problem hiding this comment.
This violates checklist item #1: "Method's name should start with a verb". There seems to be a typo in the method name. It should be handleClick to start with a verb and follow common naming conventions.
2 participants
No description provided.