mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.04k stars 31.64k forks source link

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

Closed MatheusEli closed 1 week ago

MatheusEli commented 2 weeks ago

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.

MatheusEli commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

@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

ZeeshanTamboli commented 1 week ago

@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 commented 1 week ago

@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: https://github.com/mui/material-ui/pull/42374