solution #1419
solution #1419
Conversation
There was a problem hiding this comment.
Review Summary
Hey there! 👋 Thanks for the submission.
I've reviewed the code, and while the Tabs component looks great, there are a couple of things in src/App.jsx that need to be addressed.
- Direct DOM manipulation with
document.querySelectoranddocument.querySelectorAllis generally discouraged in React. React uses a virtual DOM to efficiently update the UI, and directly modifying the DOM can lead to inconsistencies and performance issues. Consider using React's state management to handle UI updates instead. I left some comments in the code with more details and suggestions. - Also, the code is comparing objects directly, which is not ideal. It's better to compare objects using their unique identifiers (id, title).
- Additionally, the line
const pageTitle = activeTabId ? activeTabId.split('-')[1] : '';can cause an error ifactiveTabIdis not found intabs. You can avoid this error by providing a fallback value.
Let me know if you have any questions!
✨ 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.jsx
Outdated
| const selectedTab = id.split('-'); | ||
| const title = document.querySelector('.title'); | ||
| const tabContent = document.querySelector('.block'); | ||
| const removeLiClass = document.querySelectorAll('[data-cy="Tab"]'); // poderia ser 'li' |
There was a problem hiding this comment.
It's not recommended to directly manipulate the DOM using document.querySelector in React components. Instead, you should rely on React's state management to handle changes in the UI. Consider using React's state to manage the active tab and update the content accordingly.
Also, using querySelectorAll to select all elements with data-cy="Tab" and then removing the is-active class from each of them can lead to unexpected behavior if other elements on the page have the same data-cy attribute. It's better to use React's state to manage the active tab and update the class names of the tabs based on the state.
See the task description for more details.
src/App.jsx
Outdated
| const tabContent = document.querySelector('.block'); | ||
| const removeLiClass = document.querySelectorAll('[data-cy="Tab"]'); // poderia ser 'li' | ||
|
|
||
| title.textContent = `Selected tab is Tab ${selectedTab[1]}`; |
There was a problem hiding this comment.
Modifying the DOM directly like this bypasses React's virtual DOM and can lead to inconsistent UI updates and performance issues. It's better to manage the content using React's state.
See the task description for more details.
src/App.jsx
Outdated
| const removeLiClass = document.querySelectorAll('[data-cy="Tab"]'); // poderia ser 'li' | ||
|
|
||
| title.textContent = `Selected tab is Tab ${selectedTab[1]}`; | ||
| tabContent.textContent = content; |
There was a problem hiding this comment.
Modifying the DOM directly like this bypasses React's virtual DOM and can lead to inconsistent UI updates and performance issues. It's better to manage the content using React's state.
See the task description for more details.
src/App.jsx
Outdated
| li.classList.remove('is-active'); | ||
| }); | ||
|
|
||
| clickedLi.classList.add('is-active'); |
There was a problem hiding this comment.
Modifying the DOM directly like this bypasses React's virtual DOM and can lead to inconsistent UI updates and performance issues. It's better to manage the content using React's state.
See the task description for more details.
src/App.jsx
Outdated
| clickedLi.classList.add('is-active'); | ||
| }; | ||
|
|
||
| activeTabId = tabs.find(tab => tab.id === activeTabId)?.id || tabs[0].id; |
There was a problem hiding this comment.
If activeTabId is not found in tabs, this line will cause an error because you are trying to call split method for undefined value.
You can avoid this error by providing a fallback value:
const pageTitle = activeTabId ? activeTabId.split('-')[1] : '';
There was a problem hiding this comment.
Review Summary
Great job! 🎉 The code looks good overall, and you've successfully addressed the previous issues. The Tabs component is implemented correctly, rendering tabs, handling the active state, and calling the onTabSelected callback as expected. There are no specific issues that need immediate attention. Keep up the excellent work! I'm giving this an APPROVE to help you move forward. Please make sure to review the comments to continue improving the code.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
DEMO LINK