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

[Tabs] Throw error only if individual Tab is hidden, not the whole Tabs #34026

Merged
merged 9 commits into from
Jan 9, 2023

Conversation

Ryczko
Copy link
Contributor

@Ryczko Ryczko commented Aug 21, 2022

Warning conditions changed to only display when tab is not part of the document layout or is set to display: none (#33606)

Closes #33606

@mui-bot
Copy link

mui-bot commented Aug 21, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-34026--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against 89bad01

@Ryczko Ryczko closed this Aug 21, 2022
@Ryczko Ryczko deleted the tabs-Fix-nested-tabs-error branch August 21, 2022 19:29
@Ryczko Ryczko restored the tabs-Fix-nested-tabs-error branch August 21, 2022 19:30
@Ryczko Ryczko reopened this Aug 21, 2022
tabMeta &&
tabMeta.width === 0 &&
tabMeta.height === 0
tab &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not implementing the fix mentioned in the PR. Can you update the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Here is link to the implementation, that doesn't cause console error (https://codesandbox.io/s/relaxed-browser-ekxg7m). Suggested condition in the issue is incorrect, because it's breaking case, where tab item has set display: none- the warning then does not appear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution you have is hardcoded to just check the style attribute. There are cases where some classname is used for this behavior. This is why it's better to check the width and height of the element rendered.

If something is not working, is better to just commit the changes and we can take a look together at what needs to be fixed. Without seeing what's breaking it's hard to follow up :)

@oliviertassinari oliviertassinari added the component: tabs This is the name of the generic UI component, not the React module! label Aug 22, 2022
@Ryczko Ryczko requested a review from hbjORbj as a code owner August 24, 2022 15:45
@hbjORbj hbjORbj added the bug 🐛 Something doesn't work label Dec 28, 2022
@hbjORbj
Copy link
Member

hbjORbj commented Dec 29, 2022

@mnajdova I wrapped this up. Can you do a review?

@hbjORbj hbjORbj changed the title [Tabs] Fix nested tabs console error [Tabs] Throw error only if individual Tab is hidden, not the whole Tabs Dec 29, 2022
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaldudak michaldudak merged commit c0583ea into mui:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tabs] Nested tabs cause console error
6 participants