task solution 01 #1415
task solution 01 #1415
Conversation
There was a problem hiding this comment.
Review Summary
Hey there! 👋 Thanks for the submission! There are a couple of things we need to address before merging. The href attribute is hardcoded to #tab-1, but it should be dynamically set to #tab-id for each tab. Also, comparing objects directly ( activeTab === tab) can lead to issues. The checklist suggests comparing objects using their unique identifiers instead. Comparing by id would be more reliable. 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.jsx
Outdated
| <div className="section"> | ||
| <h1 className="title">Selected tab is Tab 1</h1> | ||
| export const App = () => { | ||
| const [activeTab, setActiveTab] = useState(tabs[0]); |
There was a problem hiding this comment.
It's good that you're initializing the activeTab state with the first tab. However, comparing objects directly (as you might be doing in the TabElement component) can lead to issues. The checklist suggests comparing objects using their unique identifiers instead. Please, make sure you compare tabs by id instead of the whole object.
GOOD EXAMPLE:
if (cat.id === selectedCat.id) {
| {tabs.map(tab => ( | ||
| <li | ||
| className={cn({ | ||
| 'is-active': activeTab === tab, |
There was a problem hiding this comment.
Comparing objects directly ( activeTab === tab) can lead to issues. The checklist suggests comparing objects using their unique identifiers instead. Comparing by id would be more reliable.
GOOD EXAMPLE:
if (cat.id === selectedCat.id) {
| data-cy="Tab" | ||
| key={tab.id} | ||
| > | ||
| <a href="#tab-1" data-cy="TabLink" onClick={() => setActiveTab(tab)}> |
There was a problem hiding this comment.
The href attribute is hardcoded to #tab-1. According to the task description, it should be dynamically set to #tab-id for each tab.
- Add a link with
data-cy="TabLink"and ahrefto#tab-idinside every<li>(see the markup).
There was a problem hiding this comment.
Review Summary
Okay, here's the review decision. I'm approving this pull request so you can move forward, but please pay close attention to the following feedback to improve your code. Remember, the goal is to learn and build robust applications!
Here's a breakdown of the key points:
-
hrefAttribute inTabElement.jsx: Thehrefattribute needs to be dynamically set to#tab-idfor each tab. Currently, it appears to be hardcoded based onactiveTabId. It should be:href={#${tab.id}}. This is important for proper navigation and tab selection. -
Object Comparison in
TabElement.jsx: Avoid direct object comparison (tab === activeTabId). This can be unreliable in JavaScript/React due to how object references work. Instead, compare the unique identifiers (IDs) of the objects. For example:tab.id === activeTabId.id. This is crucial for accurate tab activation logic.GOOD EXAMPLE: ```tsx if (cat.id === selectedCat.id) { -
onTabSelectedFunction Call: TheonTabSelectedfunction should only be called when the user selects a non-active tab. Add a condition to check if the tab is already active before calling the function. This aligns with the task description's requirements and prevents unnecessary state updates. -
Initialization of
activeTabIdinApp.jsx: Initializing theactiveTabIdstate variable with theidof the first tab can prevent potential errors (like trying to access.titleon an undefined value) when the component initially renders. This ensures a smoother user experience and avoids unexpected crashes.
While these points need attention, your code is generally well-structured. Addressing these issues will make your application more robust and easier to maintain. Keep up the good work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <div className="section"> | ||
| <h1 className="title">Selected tab is Tab 1</h1> | ||
| export const App = () => { | ||
| const [activeTabId, setactiveTabId] = useState(''); |
There was a problem hiding this comment.
It's a good practice to initialize the activeTabId state variable with the id of the first tab to prevent the title from being undefined initially. This will ensure that activeTabId.title does not cause an error when the component first renders.
| {tabs.map(tab => ( | ||
| <li | ||
| className={cn({ | ||
| 'is-active': tab === activeTabId, |
There was a problem hiding this comment.
To avoid potential bugs, you should compare objects using their unique identifiers (id). The checklist suggests this approach.
GOOD EXAMPLE:
```tsx
if (cat.id === selectedCat.id) {
| key={tab.id} | ||
| > | ||
| <a | ||
| href={`#${activeTabId.id}`} |
There was a problem hiding this comment.
The href attribute should be dynamically set to #tab-id for each tab, not the active tab's ID. This is a requirement from task description.
It should be: href={#${tab.id}}
| <a | ||
| href={`#${activeTabId.id}`} | ||
| data-cy="TabLink" | ||
| onClick={() => onTabSelected(tab)} |
There was a problem hiding this comment.
Call onTabSelected only when the user selects a non-active tab. This is a requirement from task description. Add a condition to check if the tab is already active before calling onTabSelected.
No description provided.