mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.51k stars 32.19k forks source link

[Tooltip] Tooltip (as slider `ValueLabel`) won't show up until I click the slider thumb #31198

Open yzhang-eightfold opened 2 years ago

yzhang-eightfold commented 2 years ago

Duplicates

Latest version

Current behavior šŸ˜Æ

When overriding ValueLabel in Slider props with a custom tooltip like this

function ValueLabelComponent(props) {
  const { children, value } = props;

  return (
    <Tooltip enterTouchDelay={0} placement="top" title={value}>
      {children}
    </Tooltip>
  );
}

we observed following behavior

Expected behavior šŸ¤”

We should not have to click on the thumb to make tooltip work

Steps to reproduce šŸ•¹

Steps:

  1. Open the sandbox of this demo https://mui.com/components/slider/#customization
  2. Verify that the 3rd slider's overridden tooltip works properly
  3. Override the same ValueLabelComponent as ValueLabel in the 4th slider(airbnbSlider)'s components prop
  4. should observe that the tooltip on the 4th slider won't show up until you click on the thumb

Context šŸ”¦

No response

Your environment šŸŒŽ

`npx @mui/envinfo` ``` "@mui/material": "^5.1.0" Microsoft Edge browser ```
hbjORbj commented 2 years ago

This issue has been fixed in https://github.com/mui/material-ui/pull/32321.

For your information, here are Codesandboxes before and after.

Codesandbox Before The Fix Codesandbox After The Fix

oliviertassinari commented 2 years ago

As far as I know, this is not a bug, it's a support question. The reproduction provided for the issue https://codesandbox.io/s/customizedslider-demo-material-ui-forked-vh93v2?file=/demo.tsx raise what is going wrong:

Screenshot 2022-07-20 at 16 55 47

The ref should be forwarded:

 interface AirbnbThumbComponentProps extends React.HTMLAttributes<unknown> {}

-function AirbnbThumbComponent(props: AirbnbThumbComponentProps) {
-  const { children, ...other } = props;
-  return (
-    <SliderThumb {...other}>
-      {children}
-      <span className="airbnb-bar" />
-      <span className="airbnb-bar" />
-      <span className="airbnb-bar" />
-    </SliderThumb>
-  );
-}
+const AirbnbThumbComponent = React.forwardRef(
+  (props: AirbnbThumbComponentProps, ref) => {
+    const { children, ...other } = props;
+    return (
+      <SliderThumb {...other} ref={ref}>
+        {children}
+        <span className="airbnb-bar" />
+        <span className="airbnb-bar" />
+        <span className="airbnb-bar" />
+      </SliderThumb>
+    );
+  }
+);

 export default function CustomizedSlider() {
   return (

once the change is done, it now works correctly: https://codesandbox.io/s/customizedslider-demo-material-ui-forked-yn4d9n?file=/demo.tsx:1276-1296.

I have reverted #32321 in b786ec4fca02cd7deec485277fb07ceb9b58f2c5 as https://github.com/mui/material-ui/pull/32321#issuecomment-1180402551 has no feedback after a week.

oliviertassinari commented 2 years ago

Regarding the next step, I think that we have two possible opportunities to better solve the root problem:

  1. We explicitly add the support for tooltip children that don't forward the ref, as a fallback mechanism. But in this case, this issue is a "new feature", the PR label would need to change. We also need a follow-up to replace the added test case to cover the use case.
  2. Or, and this is what I think makes more sense, we move more incrementally. We update the demos in the docs (https://mui.com/material-ui/react-slider/#customization) to forward the ref, so that other developers won't face the same problem when trying to be creative. The demo would feel more composable.
mnajdova commented 2 years ago

Or, and this is what I think makes more sense, we move more incrementally. We update the demos in the docs (https://mui.com/material-ui/react-slider/#customization) to forward the ref, so that others won't face the same problem. It would feel more composable.

+1 for doing this.