add solution #1319
add solution #1319
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. The getSelectedTabTitle function should return the title of the tab, not the entire tab object, which is crucial for the functionality. Additionally, there's a typo in the function name checkAvailiableTabId that should be corrected to checkAvailableTabId. Lastly, please review the necessity of the selectedTab prop in the Tabs component, as it is not mentioned in the task description. Once these issues are resolved, 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.jsx
Outdated
| Some text 1 | ||
| </div> | ||
| function getSelectedTabTitle(tabsList, tabId) { | ||
| return tabsList.find(tab => tab.id === tabId); |
There was a problem hiding this comment.
The getSelectedTabTitle function should return the title of the tab, not the entire tab object. Modify the function to return tab.title instead of the whole tab object.
src/App.jsx
Outdated
| tabs={tabs} | ||
| activeTabId={activeTabId} | ||
| onTabSelected={tabId => setNewActiveTab(tabId)} | ||
| selectedTab={selectedTab} |
There was a problem hiding this comment.
The selectedTab prop is not mentioned in the task description as a required prop for the Tabs component. Ensure that this prop is necessary, and consider removing it if it's not needed.
src/components/Tabs/Tabs.jsx
Outdated
|
|
||
| export const Tabs = ({ tabs, activeTabId, onTabSelected }) => { | ||
| const getSelectedTabTitle = (tabsList, tabId) => { | ||
| return tabsList.find(tab => tab.id === tabId); |
There was a problem hiding this comment.
The getSelectedTabTitle function should return the title of the tab, not the entire tab object. Modify the function to return tab.title instead of the whole tab object.
src/components/Tabs/Tabs.jsx
Outdated
| return tabsList.find(tab => tab.id === tabId); | ||
| }; | ||
|
|
||
| function checkAvailiableTabId(id) { |
There was a problem hiding this comment.
There's a typo in the function name checkAvailiableTabId. Consider renaming it to checkAvailableTabId for correct spelling.
There was a problem hiding this comment.
Great job on addressing the issues from the previous review! 🎉 The code now meets the task requirements and constraints, and the implementation follows the specified guidelines for rendering tabs and managing active states. Here are some important points to consider:
-
getSelectedTabTitleFunction: You've successfully corrected the function to return only the title of the tab, which resolves the previous issue. -
Function Naming: The typo in the function name
checkAvailiableTabIdhas been corrected tocheckAvailableTabId. Ensure that all references to the old name are removed, including any commented-out lines. -
selectedTabVariable: The necessity of theselectedTabvariable in theAppcomponent was questioned. Please review its usage and remove it if it's not required according to the task description. -
Code Cleanliness: Keep your code clean by removing unnecessary comments and ensuring that all references to outdated code are eliminated.
Your efforts to improve the code are evident, and this decision is made to help you move forward with your learning. Please review the comments to further enhance your code quality. Keep up the great work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| export const App = () => { | ||
| const [activeTabId, setActiveTabId] = useState(tabs[0].id); | ||
| const selectedTab = getSelectedTabTitle(tabs, activeTabId); |
There was a problem hiding this comment.
The selectedTab variable is being used here, but its necessity was questioned in the previous review. Please ensure that this variable is required according to the task description, or remove it if it's not needed.
| key={tab.id} | ||
| tab={tab} | ||
| activeTabId={checkAvailableTabId(activeTabId)} | ||
| // activeTabId={checkAvailiableTabId(activeTabId)} |
There was a problem hiding this comment.
Remove this commented-out line as it references the old function name checkAvailiableTabId, which has been corrected to checkAvailableTabId. Keeping the code clean and free of unnecessary comments is a good practice.
DEMO LINK