Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The misspelling of the method name handleClik should be corrected to handleClick to maintain consistency and readability. Additionally, using inline event handlers can lead to performance issues, so it's best to refactor them into separate methods. Lastly, ensure that each list item has a unique key prop to meet React's requirements for list rendering. Once these changes are made, your code will be in great shape! Keep up the great work! 🚀
✨ 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.
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> | ||
| handleClik = (good: string) => { |
There was a problem hiding this comment.
There is a typo in the method name handleClik. It should be handleClick to maintain consistency and readability.
src/App.tsx
Outdated
| type="button" | ||
| className="button" | ||
| onClick={() => { | ||
| this.handleClik(good); |
There was a problem hiding this comment.
Ensure that the method name handleClik is corrected to handleClick here as well.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Great job on addressing most of the issues from the previous review! 🎉 Your code is well on its way to meeting all the requirements. Here are some points to consider for further improvement:
-
Unique Key Prop: Ensure that each list item in the
newTableGoodsarray has a uniquekeyprop. This is crucial for React's list rendering efficiency. You might use thegoodvalue or its index as thekey. -
Inline Event Handlers: Although you've made progress, there's still an inline event handler in the
onClickattribute of theAddButton. Refactoring this into a separate method will enhance code readability and maintainability.
Your solution meets the task requirements, handling the selection and clearing of goods effectively, and managing button visibility based on the selection state. Keep up the good work, and make sure to address the highlighted issues in future iterations to enhance your React skills. Remember, learning from these experiences will make you a better developer. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| const newTableGoods = goods.map(good => { | ||
| return good === selectedGood ? ( | ||
| <tr data-cy="Good" className="has-background-success-light"> | ||
| <td> | ||
| <button | ||
| data-cy="RemoveButton" | ||
| type="button" | ||
| className="button is-info" | ||
| onClick={this.clearClick} | ||
| > | ||
| - | ||
| </button> | ||
| </td> | ||
|
|
||
| <td data-cy="GoodTitle" className="is-vcentered"> | ||
| Jam | ||
| {good} | ||
| </td> | ||
| </tr> | ||
|
|
||
| ) : ( | ||
| <tr data-cy="Good"> | ||
| <td> | ||
| <button data-cy="AddButton" type="button" className="button"> | ||
| <button | ||
| data-cy="AddButton" | ||
| type="button" | ||
| className="button" | ||
| onClick={() => { | ||
| this.handleClick(good); | ||
| }} | ||
| > | ||
| + | ||
| </button> | ||
| </td> | ||
|
|
||
| <td data-cy="GoodTitle" className="is-vcentered"> | ||
| Garlic | ||
| {good} | ||
| </td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| </main> | ||
| ); | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Each element in the newTableGoods array should have a unique key prop. This is important for React to efficiently update and render list items. Consider using the good value or its index in the array as the key.
2 participants
[DEMO LINK] (https://Mateuszcieplak.github.io/react_goods-selector/)