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

[material-ui][docs] Fix the Basic Tabs demo #42253

Closed
wants to merge 3 commits into from

Conversation

MatheusEli
Copy link
Contributor

@MatheusEli MatheusEli commented May 15, 2024

To avoid validateDOMNesting warning, we can use span as a component of Typography instead of p (default component).

A p tag can only contain inline elements. That means putting a div tag inside it should be improper, since the div tag is a block element. Improper nesting might cause glitches like rendering extra tags, which can affect your javascript and css.

To avoid validateDOMNesting warning, we can use span as a component of Typography instead of p (default component). 

A <p></p> tag can only contain inline elements. That means putting a <div></div> tag inside it should be improper, since the div tag is a block element. Improper nesting might cause glitches like rendering extra tags, which can affect your javascript and css.

Signed-off-by: Matheus Eli <31008177+MatheusEli@users.noreply.github.com>
@zannager zannager added docs Improvements or additions to the documentation component: tabs This is the name of the generic UI component, not the React module! labels May 16, 2024
@zannager zannager requested a review from mnajdova May 16, 2024 10:44
@danilo-leal danilo-leal changed the title [Tabs][docs] Fix BasicTabs.js Typography Component [material-ui][docs] Fix the Basic Tabs demo May 18, 2024
@danilo-leal danilo-leal added the package: material-ui Specific to @mui/material label May 18, 2024
Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Signed-off-by: Matheus Eli <31008177+MatheusEli@users.noreply.github.com>
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@MatheusEli You're right that a div shouldn't be inside a p, but this demo doesn't include a div child in the rendered Typography. If your app uses a div child, you should add component="span". Adding it in this demo is unnecessary.

@MatheusEli
Copy link
Contributor Author

@MatheusEli You're right that a div shouldn't be inside a p, but this demo doesn't include a div child in the rendered Typography. If your app uses a div child, you should add component="span". Adding it in this demo is unnecessary.

I agree @ZeeshanTamboli but i've seen a lot of people getting these warnings because you usually will put some tags and components inside tabs, maybe this will help them not getting that? like @qkeddy said in #21015

@ZeeshanTamboli
Copy link
Member

@MatheusEli You're right that a div shouldn't be inside a p, but this demo doesn't include a div child in the rendered Typography. If your app uses a div child, you should add component="span". Adding it in this demo is unnecessary.

I agree @ZeeshanTamboli but i've seen a lot of people getting these warnings because you usually will put some tags and components inside tabs, maybe this will help them not getting that? like @qkeddy said in #21015

I understand your point after reading the comments in issue #21015. The issue is that users don't realize Typography renders a p tag, leading to warnings when they copy and paste the demo code into their applications. Still changing the underlying component to span in this demo doesn't make sense because the tab panel only renders text. A better solution might be to remove the Typography component from the demo:

--- a/docs/data/material/components/tabs/BasicTabs.tsx
+++ b/docs/data/material/components/tabs/BasicTabs.tsx
@@ -1,7 +1,6 @@
 import * as React from 'react';
 import Tabs from '@mui/material/Tabs';
 import Tab from '@mui/material/Tab';
-import Typography from '@mui/material/Typography';
 import Box from '@mui/material/Box';

 interface TabPanelProps {
@@ -21,11 +20,7 @@ function CustomTabPanel(props: TabPanelProps) {
       aria-labelledby={`simple-tab-${index}`}
       {...other}
     >
-      {value === index && (
-        <Box sx={{ p: 3 }}>
-          <Typography>{children}</Typography>
-        </Box>
-      )}
+      {value === index && <Box sx={{ p: 3 }}>{children}</Box>}
     </div>
   );
 }

@MatheusEli
Copy link
Contributor Author

@MatheusEli You're right that a div shouldn't be inside a p, but this demo doesn't include a div child in the rendered Typography. If your app uses a div child, you should add component="span". Adding it in this demo is unnecessary.

I agree @ZeeshanTamboli but i've seen a lot of people getting these warnings because you usually will put some tags and components inside tabs, maybe this will help them not getting that? like @qkeddy said in #21015

I understand your point after reading the comments in issue #21015. The issue is that users don't realize Typography renders a p tag, leading to warnings when they copy and paste the demo code into their applications. Still changing the underlying component to span in this demo doesn't make sense because the tab panel only renders text. A better solution might be to remove the Typography component from the demo:

--- a/docs/data/material/components/tabs/BasicTabs.tsx
+++ b/docs/data/material/components/tabs/BasicTabs.tsx
@@ -1,7 +1,6 @@
 import * as React from 'react';
 import Tabs from '@mui/material/Tabs';
 import Tab from '@mui/material/Tab';
-import Typography from '@mui/material/Typography';
 import Box from '@mui/material/Box';

 interface TabPanelProps {
@@ -21,11 +20,7 @@ function CustomTabPanel(props: TabPanelProps) {
       aria-labelledby={`simple-tab-${index}`}
       {...other}
     >
-      {value === index && (
-        <Box sx={{ p: 3 }}>
-          <Typography>{children}</Typography>
-        </Box>
-      )}
+      {value === index && <Box sx={{ p: 3 }}>{children}</Box>}
     </div>
   );
 }

You're right, removing the Typography component from the demo is a better solution

Signed-off-by: Matheus Eli <31008177+MatheusEli@users.noreply.github.com>
@ZeeshanTamboli
Copy link
Member

@MatheusEli, I noticed your branch is pointing to master. The default branch is next. Could you please create a new PR and point it to next? Also, make sure to update the corresponding TypeScript file for the demo (BasicTabs.tsx).

@MatheusEli
Copy link
Contributor Author

@MatheusEli, I noticed your branch is pointing to master. The default branch is next. Could you please create a new PR and point it to next? Also, make sure to update the corresponding TypeScript file for the demo (BasicTabs.tsx).

Sure! here it is: #42374

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants