fix(tab): out of bounds issue for screen size change #226
Conversation
|
There was a problem hiding this comment.
Hi @tatata96 , I realized code formatting on your PRs are a bit weird. Can you make sure your VSCode/IDE is using the correct linter/formatter?
For example, when I save this file, I get this diff (it formats according to our linter config files):
Your ide should be using the local prettier config
| @@ -0,0 +1,28 @@ | |||
| import {useState, useEffect} from "react"; | |||
There was a problem hiding this comment.
using useWindowSize (this will be removed, only there for testing purposes).
I saw this part, but this hook is not actually used at the moment, so maybe you can remove from remote, but keep it on your local if you need it
We could add an example of changing items to the story, but I don't think that's necessary
There was a problem hiding this comment.
I added that for testing purposes, it will be removed before the pr is merged
There was a problem hiding this comment.
Not related to that issue but it came to mind while thinking about it,
Maybe we can simplify the handleChangeActiveTab function.
Here is my suggestion:
const tabChangeHandler = onTabChange || setActiveTabIndex;
function handleChangeActiveTab(index: number) {
if (index !== activeTabIndexToUse) {
tabChangeHandler(index);
}
}What do you think? @tatata96 @Hipo/frontend-team
src/tab/Tab.tsx
Outdated
| let activeTabIndexToUse = activeTabIndexFromProps === undefined | ||
| ? activeTabIndex | ||
| : activeTabIndexFromProps; |
There was a problem hiding this comment.
Would that work?
| let activeTabIndexToUse = activeTabIndexFromProps === undefined | |
| ? activeTabIndex | |
| : activeTabIndexFromProps; | |
| let activeTabIndexToUse = activeTabIndexFromProps || activeTabIndex |
There was a problem hiding this comment.
What if activeTabIndexFromProps is 0? Does it still work as expected? This may be more reliable:
let activeTabIndexToUse = activeTabIndexFromProps ?? activeTabIndex;
|
I think yours is more readable but it may cause issues for cases where we want to use
What do you think? @Anlerkan |
| let activeTabIndexToUse = activeTabIndexFromProps || activeTabIndex; | ||
|
|
||
| /** | ||
| * In case activeTabIndex is out of bounds, we need to reset it to 0. | ||
| * This can happen if items are changes after the mount | ||
| */ | ||
| if (activeTabIndexToUse > items.length) { | ||
| activeTabIndexToUse = 0; | ||
| } |
There was a problem hiding this comment.
This should be reflected to the state and also we should call onTabChange if the Tab is being used as a controlled component
Related Issue;
I tried recreate problem by updating the
items(it had 5 elements, I changed it to 2) for screens smaller than 400, usinguseWindowSize(this will be removed, only there for testing purposes).Screen.Recording.2023-01-26.at.22.38.00.mov
After adding the out of bounds check and setting
indexto 0:Screen.Recording.2023-01-26.at.23.02.23.mov