[Slider] Custom tooltip ignores valueLabelFormat and valueLabelDisplay

[Floriferous]

When you use the Slider with a custom ValueLabelComponent, it ignores the passed valueLabelFormat and valueLabelDisplay.

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When you provide a custom component it gets the raw number value as a prop, instead of the formatted one. It also doesn't display according to valueLabelDisplay.

Expected Behavior 🤔

I expect the value prop passed to the ValueLabelComponent to be formatted by the valueLabelFormat.

I also expect the valueLabelDisplay value to impact whether the ValueLabelComponent is displayed or not.

Steps to Reproduce 🕹

https://codesandbox.io/s/create-react-app-0jbd4

Your Environment 🌎

Tech	Version
Material-UI	v4.5.1
React	v16.10.2
Browser	Chrome

[mark-tate]

That's not how the API works, this API enables you to replace the default ValueLabel with your own.

In your codesandbox's FormatTooltip, log out the props and you will see that you have valueLabelDisplay and valueLabelFormat.

You just need to call title={valueLabelFormat(value)} and then use valueLabelDisplay to determine what component is returned.

Refer again to
ValueLabel.js for an example.

[oliviertassinari]

1. I expect the value prop passed to the ValueLabelComponent to be formatted by the valueLabelFormat.

@Floriferous I agree, this sounds like a valid request. It's the only use case for the valueLabelFormat prop. If somebody provides value different from the default identity function, I think that we can as well use it.

2. I also expect the valueLabelDisplay value to impact whether the ValueLabelComponent is displayed or not.

No objection either, we could move the logic to the parent. This makes sense.

In practice, I think that the changes will look something like:

diff --git a/docs/src/pages/components/slider/CustomizedSlider.tsx b/docs/src/pages/components/slider/CustomizedSlider.tsx
index e141773f7..230822ca3 100644
--- a/docs/src/pages/components/slider/CustomizedSlider.tsx
+++ b/docs/src/pages/components/slider/CustomizedSlider.tsx
@@ -210,6 +210,7 @@ export default function CustomizedSlider() {
       <div className={classes.margin} />
       <Typography gutterBottom>Tooltip value label</Typography>
       <Slider
+        valueLabelDisplay="auto"
         ValueLabelComponent={ValueLabelComponent}
         aria-label="custom thumb label"
         defaultValue={20}
diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 883e0380a..7d294384f 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -315,6 +315,8 @@ export const styles = theme => ({
   },
 });

+const Forward = ({ children }) => children;
+
 const Slider = React.forwardRef(function Slider(props, ref) {
   const {
     'aria-label': ariaLabel,
@@ -340,7 +342,7 @@ const Slider = React.forwardRef(function Slider(props, ref) {
     ThumbComponent = 'span',
     track = 'normal',
     value: valueProp,
-    ValueLabelComponent = ValueLabel,
+    ValueLabelComponent: ValueLabelComponentProp = ValueLabel,
     valueLabelDisplay = 'off',
     valueLabelFormat = Identity,
     ...other
@@ -745,6 +747,9 @@ const Slider = React.forwardRef(function Slider(props, ref) {
       {values.map((value, index) => {
         const percent = valueToPercent(value, min, max);
         const style = axisProps[axis].offset(percent);
+        const ValueLabelComponent = valueLabelDisplay === 'off' ? Forward : ValueLabelComponentProp;
+

         return (
           <ValueLabelComponent
@@ -752,9 +757,13 @@ const Slider = React.forwardRef(function Slider(props, ref) {
             valueLabelFormat={valueLabelFormat}
             valueLabelDisplay={valueLabelDisplay}
             className={classes.valueLabel}
-            value={value}
+            value={
+              typeof valueLabelFormat === 'function'
+                ? valueLabelFormat(value, index)
+                : valueLabelFormat
+            }
             index={index}
-            open={open === index || active === index}
+            open={open === index || active === index || valueLabelDisplay === 'on'}
             disabled={disabled}
           >
             <ThumbComponent
diff --git a/packages/material-ui/src/Slider/ValueLabel.js b/packages/material-ui/src/Slider/ValueLabel.js
index e8729a660..a9b47c987 100644
--- a/packages/material-ui/src/Slider/ValueLabel.js
+++ b/packages/material-ui/src/Slider/ValueLabel.js
@@ -45,20 +45,7 @@ const styles = theme => ({
  * @ignore - internal component.
  */
 function ValueLabel(props) {
-  const {
-    children,
-    classes,
-    className,
-    index,
-    open,
-    value,
-    valueLabelDisplay,
-    valueLabelFormat,
-  } = props;
-
-  if (valueLabelDisplay === 'off') {
-    return children;
-  }
+  const { children, classes, className, open, value } = props;

   return React.cloneElement(
     children,
@@ -66,18 +53,14 @@ function ValueLabel(props) {
       className: clsx(
         children.props.className,
         {
-          [classes.open]: open || valueLabelDisplay === 'on',
+          [classes.open]: open,
         },
         classes.thumb,
       ),
     },
     <span className={clsx(classes.offset, className)}>
       <span className={classes.circle}>
-        <span className={classes.label}>
-          {typeof valueLabelFormat === 'function'
-            ? valueLabelFormat(value, index)
-            : valueLabelFormat}
-        </span>
+        <span className={classes.label}>{value}</span>
       </span>
     </span>,
   );

I'm wondering if we should consider these changes breaking. I have a concern with the valueLabelDisplay default value. I would propose that we handle 1. now, and we defer 2 to v5

@Floriferous Do you want to handle part of the issue? :)

[kvin97]

Shall I work on this issue ?

[oliviertassinari]

@kvin97 You are free to go :), please only handle the problem n°1 ("I expect the value prop passed to the ValueLabelComponent to be formatted by the valueLabelFormat."). I suspect that n°2 would be breaking.