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

Remove state from Tabs component #220

Closed
benjitrosch opened this issue Sep 23, 2022 · 1 comment · Fixed by #226
Closed

Remove state from Tabs component #220

benjitrosch opened this issue Sep 23, 2022 · 1 comment · Fixed by #226
Labels
good first issue Good for newcomers

Comments

@benjitrosch
Copy link
Collaborator

benjitrosch commented Sep 23, 2022

Our current <Tabs> parent component maintains the active tab value in a useState and passes the value along to all of the child <Tab> components.

https://github.com/daisyui/react-daisyui/blob/main/src/Tabs/Tabs.tsx

However, this internal state-keeping causes problems when we attempt to control Tabs from the outside using the value prop. It would be more correct to call the value prop a defaultValue since useState does not reload the initial value from props on rerender.

The way I see it, there are couple ways forward with this:

  1. Add a useEffect hook with a dependency on the value prop to update the internal state. (Not a huge fan of this solution)
  2. Remove the internal state entirely, and let the user control the state themselves using the exposed onChange prop.

I'll leave the solution up to whoever wants to tackle this. Open to other ideas as well!

@benjitrosch benjitrosch added the good first issue Good for newcomers label Sep 23, 2022
@chargome
Copy link
Contributor

chargome commented Oct 5, 2022

Hey @benjitrosch, can you assign the issue to me?

I looked into it and I think that handling the state outside of the <Tabs> component through the value and onChange props would make more sense and feel more intuitive.

chargome added a commit to chargome/react-daisyui that referenced this issue Oct 5, 2022
benjitrosch pushed a commit that referenced this issue Oct 6, 2022
* feat: handle tabs state externally (#220)

* test: add tests for Tabs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants