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][Tab] Fix applying iconWrapper styles from theme and update its description #42549

Merged
merged 12 commits into from
Jun 8, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Jun 6, 2024

Bug

According to docs, styles applied passed to iconWrapper should be applied to wrapper element of icon, in this case it is TabRoot component. But styles are not getting applied as we can see in below before sandbox

Before: https://stackblitz.com/edit/react-kgd9jj?file=Demo.tsx (background color should be red according theme but that's not the case)

After: Replace code in this demo with below code to see effects of fix

import * as React from 'react';
import Tabs from '@mui/material/Tabs';
import Tab from '@mui/material/Tab';
import Box from '@mui/material/Box';
import { ThemeProvider, createTheme } from '@mui/material/styles';

interface TabPanelProps {
  children?: React.ReactNode;
  index: number;
  value: number;
}

function CustomTabPanel(props: TabPanelProps) {
  const { children, value, index, ...other } = props;

  return (
    <div
      role="tabpanel"
      hidden={value !== index}
      id={`simple-tabpanel-${index}`}
      aria-labelledby={`simple-tab-${index}`}
      {...other}
    >
      {value === index && <Box sx={{ p: 3 }}>{children}</Box>}
    </div>
  );
}

function a11yProps(index: number) {
  return {
    id: `simple-tab-${index}`,
    'aria-controls': `simple-tabpanel-${index}`,
  };
}

export default function BasicTabs() {
  const [value, setValue] = React.useState(0);

  const handleChange = (event: React.SyntheticEvent, newValue: number) => {
    setValue(newValue);
  };
  const theme = createTheme({
    components: {
      MuiTab: {
        styleOverrides: {
          iconWrapper: {
            background: 'red',
          },
        },
      },
    },
  });
  return (
    <ThemeProvider theme={theme}>
      <Box sx={{ width: '100%' }}>
        <Box sx={{ borderBottom: 1, borderColor: 'divider' }}>
          <Tabs value={value} onChange={handleChange} aria-label="basic tabs example">
            <Tab icon="icon   " label="Item One" {...a11yProps(0)} />
            <Tab label="Item Two" {...a11yProps(1)} />
            <Tab label="Item Three" {...a11yProps(2)} />
          </Tabs>
        </Box>
        <CustomTabPanel value={value} index={0}>
          Item One
        </CustomTabPanel>
        <CustomTabPanel value={value} index={1}>
          Item Two
        </CustomTabPanel>
        <CustomTabPanel value={value} index={2}>
          Item Three
        </CustomTabPanel>
      </Box>
    </ThemeProvider>
  );
}

@sai6855 sai6855 added component: tabs This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Jun 6, 2024
@sai6855 sai6855 marked this pull request as draft June 6, 2024 10:44
@mui-bot
Copy link

mui-bot commented Jun 6, 2024

Netlify deploy preview

https://deploy-preview-42549--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fe9a07c

@sai6855 sai6855 marked this pull request as ready for review June 6, 2024 13:17
@sai6855 sai6855 added the bug 🐛 Something doesn't work label Jun 6, 2024
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.

This is incorrect. In the code, the iconWrapper class is applied to the icon itself when both icon (as a valid React element, not a string) and label props are present. You can also verify this in the second demo of this section in the docs.

This logic was added in #28982 to replace the extra span element wrapper with React.cloneElement and then apply the className to fix :first-child warnings.

I understand the name iconWrapper is confusing indicating a "wrapper", but we can't change the class name in v5 only in v6. What we can do is update the style class description to:

Styles applied to the `icon` HTML element if both `icon` and `label` are provided.

What do you think about changing the style class name from iconWrapper to icon for v6 in this PR? cc @DiegoAndai

@sai6855
Copy link
Contributor Author

sai6855 commented Jun 7, 2024

Agree on changing description of iconWrapper class, but what should be way forward about styles passed for iconWrapper in theme not being applied to icon.

I don't think we can access styles.iconWrapper here, so i'm not sure how to fix this.

For now, i've just updated description in this PR, we can update iconWrapper to icon in different PR if we decided to update

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jun 7, 2024

but what should be way forward about styles passed for iconWrapper in theme not being applied to icon.

We can do this:

--- a/packages/mui-material/src/Tab/Tab.js
+++ b/packages/mui-material/src/Tab/Tab.js
@@ -42,6 +42,9 @@ const TabRoot = styled(ButtonBase, {
       styles[`textColor${capitalize(ownerState.textColor)}`],
       ownerState.fullWidth && styles.fullWidth,
       ownerState.wrapped && styles.wrapped,
+      {
+        [`& .${tabClasses.iconWrapper}`]: styles.iconWrapper,
+      },
     ];
   },
 })(({ theme }) => ({

Make sure to add a test.

we can update iconWrapper to icon in different PR if we decided to update

Agree with you. And first create a new issue so that it is tracked.

@sai6855
Copy link
Contributor Author

sai6855 commented Jun 7, 2024

We can do this:

Added in these commits 988bf86 e692070, intresting solution BTW, thanks!!

@ZeeshanTamboli ZeeshanTamboli added the needs cherry-pick The PR should be cherry-picked to master after merge label Jun 7, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Tab] Fix applying iconWrapper styles from theme [material-ui][Tab] Fix applying iconWrapper styles from theme and change it's description Jun 8, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Tab] Fix applying iconWrapper styles from theme and change it's description [material-ui][Tab] Fix applying iconWrapper styles from theme and update its description Jun 8, 2024
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.

Looks good.

@ZeeshanTamboli ZeeshanTamboli merged commit f3188de into mui:next Jun 8, 2024
22 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 8, 2024
…pdate its description (#42549)

Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
@DiegoAndai
Copy link
Member

DiegoAndai commented Jun 14, 2024

Sorry I missed this

What do you think about changing the style class name from iconWrapper to icon for v6 in this PR?

I agree, but let's deprecate iconWrapper and add icon. They will be the same. @sai6855 may I ask you to open a PR for this?

@sai6855
Copy link
Contributor Author

sai6855 commented Jun 15, 2024

@sai6855 may I ask you to open a PR for this?

opened PR -> #42647

joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
…pdate its description (mui#42549)

Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
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! needs cherry-pick The PR should be cherry-picked to master after merge package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants