test_1 #1977
test_1 #1977
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 There are a few critical issues that need to be addressed before we can approve it.
-
Typing Issues: The
React.Componentgeneric type is incorrectly used, which may lead to runtime errors. It should beReact.Component<{}, State>since the component does not take any props. Additionally, thePropsinterface is being used to define the state, which is misleading. It would be clearer to renamePropstoState. -
Method Naming: The method
handleshould start with a verb to clearly indicate its action. This aligns with best practices for method naming. -
Event Listener Management: The code does not include the removal of the event listener in
componentWillUnmount. It's important to useremoveEventListenerto clean up and prevent potential memory leaks.
Please address these issues to ensure the code meets the task requirements and functions correctly. Once these changes are made, we can proceed with the approval. Keep up the great work, and don't hesitate to reach out if you have any questions! 🚀
✨ 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
| message: string; | ||
| } | ||
|
|
||
| export class App extends React.Component<{}, Props> { |
There was a problem hiding this comment.
The React.Component generic type is used incorrectly. The first type parameter should be the type of the component's props, and the second should be the type of the component's state. Since this component does not take any props, the correct usage should be React.Component<{}, Props>.
src/App.tsx
Outdated
| state: Props = { | ||
| message: 'Nothing was pressed yet', | ||
| }; |
There was a problem hiding this comment.
The Props interface is being used to define the state, which is misleading. It's better to rename Props to something like State to accurately reflect its purpose.
There was a problem hiding this comment.
Your code has been reviewed and approved! 🎉
Review Summary:
Great job on updating the code to address previous issues! The Props interface has been correctly renamed to State, and the event listener is now being removed in componentWillUnmount, which is a crucial improvement. The React.Component generic type is also correctly used with an empty props object.
However, there are still a few areas that need attention. The method handleButton should start with a verb to better reflect its action. Consider renaming it to something like handleKeyUp or onKeyUp to align with best practices for method naming.
Overall, your solution meets the task requirements and constraints. The App component is correctly implemented as a class component with a state, and the event handling is properly managed to prevent potential memory leaks. The message updates as expected when a key is pressed.
Keep up the great work, and make sure to review the comments to improve your code further. Addressing these issues will help you enhance your coding skills and avoid similar problems in the future. Keep learning and growing! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| message: 'Nothing was pressed yet', | ||
| }; | ||
|
|
||
| handleButton = (event: KeyboardEvent) => { |
There was a problem hiding this comment.
The method handleButton should start with a verb to clearly indicate its action. Consider renaming it to something like handleKeyUp or onKeyUp to align with best practices for method naming.
DEMO LINK