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] Alternative way to disable ":first-child is unsafe" error #28982

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Oct 11, 2021

This PR introduces 2 alternatives to disable the ":first-child is unsafe" error:

  • Source of error:
...(ownerState.icon &&
    ownerState.label && {
      ...,
      [`& > *:first-child`]: {
        marginBottom: 6,
      },
    }),
@@ -176,7 +183,8 @@ const Tab = React.forwardRef(function Tab(inProps, ref) {
       tabIndex={selected ? 0 : -1}
       {...other}
     >
-      {icon}
+      {icon && <span className={classes.icon}>{icon}</span>}

Since the first iteration changes the DOM rendered, I prepared two alternatives:
(1) Use '/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */'

@@ -59,8 +62,10 @@ const TabRoot = styled(ButtonBase, {
        ...,
-      [`& > *:first-child`]: {
-        marginBottom: 6,
+      [`& > *`]: {
+        [`:first-child ${ignoreSsrFlag}`]: {
+          marginBottom: 6,
+        },
       },
     }),
  • most concise solution but not supporting zero SSR configuration might not be a good idea

(2) Clone icon with a classname and target the classname for styling:

@@ -148,7 +148,12 @@ const Tab = React.forwardRef(function Tab(inProps, ref) {
   };
 
   const classes = useUtilityClasses(ownerState);
-
+  const icon =
+    iconProp && label && typeof iconProp !== 'string'
+      ? React.cloneElement(iconProp, {
+          className: classes.iconWrapper,
+        })
+      : iconProp;

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 11, 2021

Details of bundle changes

Generated by 🚫 dangerJS against f4f5d05

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Changed tests are usually an indicator that a change is breaking existing behaivor. Either make the tests purely additive, explain why this is a bugfix not a feature or delay until v6

@mnajdova
Copy link
Member

Changed tests are usually an indicator that a change is breaking existing behaivor. Either make the tests purely additive, explain why this is a bugfix not a feature or delay until v6

@eps1lon the tests changes are coming from #28890 This PR is summarizing the alternatives proposed on that PR after it was merged. I guess this is why some tests need to be adjusted (it is the tests added in the mentioned PR, not something that was implemented before).

@hbjORbj
Copy link
Member Author

hbjORbj commented Oct 11, 2021

Changed tests are usually an indicator that a change is breaking existing behaivor. Either make the tests purely additive, explain why this is a bugfix not a feature or delay until v6

@eps1lon Sorry for confusion. The tests modified in this PR hadn't existed prior to #28890 (the first iteration to address this bug). So, the tests can be considered purely additive.

@hbjORbj hbjORbj merged commit 3b406df into mui:master Oct 12, 2021
@zannager zannager added the component: tabs This is the name of the generic UI component, not the React module! label Jan 25, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants