Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for defaultTabIndex props in tab component because by default it select first tab as active tab #91

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

vpsk
Copy link
Contributor

@vpsk vpsk commented Nov 29, 2022

Add support for defaultTabIndex props in tab component because by default it select first tab as active tab

@vpsk vpsk changed the title Add support for defaultTabIndex props in tab component because by def… Add support for defaultTabIndex props in tab component because by default it select first tab as active tab Nov 29, 2022
Sreejit7
Sreejit7 previously approved these changes Nov 29, 2022
@sujan-s
Copy link
Member

sujan-s commented Nov 30, 2022

@vpsk Here’s an idea—

tabs={[
    {
        key: "tab1",
        label: "Tab 1",
        content: Tab1Content(),
    },
    {
        key: "tab2",
        label: "Tab 2",
        content: Tab2Content(),
        isActive <—— <—— 
    }
}

What say we pass an isActive attribute to whatever tab you want activated by default?

@vpsk
Copy link
Contributor Author

vpsk commented Nov 30, 2022

@sujan-s default active Index is for only the first render and it should be independent of the tab details array.

isActive in tab object will make sense to get information about the currently active tab.

ghostwriternr
ghostwriternr previously approved these changes Nov 30, 2022
@sujan-s
Copy link
Member

sujan-s commented Nov 30, 2022

@vpsk Understood. Makes sense. Here’s another thing to think about—defaultTabIndex is very dev-centric, and against the general Fictoan ideology. Can we do something like defaultActiveTab="tab1", which would be the value of the key in the tabs object.

That way, it’s plain English and self-explanatory.

Also, can you update the CHANGELOG and the version too, please? Once we merge to master, it’ll update the npm package too.

@vpsk vpsk dismissed stale reviews from ghostwriternr and Sreejit7 via 41df54c November 30, 2022 10:38
@vpsk
Copy link
Contributor Author

vpsk commented Nov 30, 2022

@sujan-s update the naming of prop and active tab based on key.

@vpsk vpsk merged commit 231ee1e into master Nov 30, 2022
@vpsk vpsk deleted the enhancement/tab-active-index branch November 30, 2022 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants