Is there a better alternative to cloneElement?

[casank25]

When using ToggleButton from lab package and wrapping it in tooltip it raises a warning:
Warning: Failed prop type: The following properties are not supported: selected, onChange. Please remove them.

• This is not a v1.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

It should be possible to add a tooltip to ToggleButton

Current Behavior

<Tooltip title="left" placement="top">
  <ToggleButton value="left">
    <FormatAlignLeftIcon />
  </ToggleButton>
</Tooltip>

Steps to Reproduce

Link: https://codesandbox.io/s/q7m82k4024

1. Wrap the ToggleButton inside a Tooltip
2. Run the code
3. See the console for warning

Context

Since ToggleButtons are IconButton (at least that's how we intend to use it) it would be helpful to a user to see what that button will do and the best way for that is Tooltip

Your Environment

Tech	Version
Material-UI	v3.1.0
React	v16.4
Browser	any

[oliviertassinari]

@casank25 Either you need to reverse the order:

              <ToggleButton value="left">
                <Tooltip title="left" placement="top">
                  <FormatAlignLeftIcon />
                </Tooltip>
              </ToggleButton>

or to write a wrapping component to forward the properties where it's needed (jumping the tooltip).

[oliviertassinari]

Actually, we could forward the unused properties to the children to make it work out of the box. Do you want to work on that?

[RobertPurcea]

Working on it, I'll submit a PR today

[casank25]

Ha, I guess I came too late to check back on this issue :)

[RobertPurcea]

Actually, this might need a little more consideration.

The problem stated by the OP is caused by ToggleButtonGroup.
This component assumes it's immediate children will be buttons and passes 'selected' and 'onChange' props.
Now, this can be solved if Tooltip would simply pass all unused props but there is a second issue.

ToggleButtonGroup uses the 'value' prop of it's children to make toggle functionality work.
Without it, toggling does not work.

    const children = React.Children.map(childrenProp, child => {
      if (!React.isValidElement(child)) {
        return null;
      }
      // value comes from children. Having an empty tooltip is breaking this
      const { selected: buttonSelected, value: buttonValue } = child.props;

      const selected =
        buttonSelected === undefined ? isValueSelected(buttonValue, value) : buttonSelected;

      return React.cloneElement(child, {
        selected,
        onChange: exclusive ? this.handleExclusiveChange : this.handleChange,
      });
    });

@oliviertassinari let me know if you still want to progress with this, I am not sure there's a good way to support what OP is asking and whether or not it's worth it

[eps1lon]

Just a random thought but wouldn't it be better to create a context in ToggleButtonGroup that the ToggleButtons can listen to? That way we could get away from the cloneElement pattern.

[oliviertassinari]

This issue is really interesting it's at the reunion of many pain points:

we could forward the unused properties to the children to make it work out of the box

• It would also solve Tooltip inside CardActions raises Warning: Failed prop type #12302. However, it's going against forwarding the properties to the tooltip label [Tooltip] How to customize the tooltip? #11467.

Just a random thought but wouldn't it be better to create a context in ToggleButtonGroup that the ToggleButtons can listen to?

• It's a discussion we started in [RadioButtonGroup] breaks when nesting non-RadioButton components #2225.

[oliviertassinari]

Regarding the cloneElement usage. @eps1lon is right, it's a broader issue. We have many components defaulting to this strategy.

• Using cloneElement has multiple drawbacks:

1. The behavior is hidden to the users. It can feel like magic. It's making wrapping component harder to write correctly. You need to do stuff like setting muiName and drilling properties down multiple components.
2. It's breaking virtualization potentials.

• Using the context API could be a way out. It's solving the virtualization issue. However, it's a double edge sword for the wrapping component story. If you don't need to access the injected properties it's perfect. If you do, it's a nightmare. Also, I'm unsure about the UX (bundle size and runtime performance overhead) and DX (enzyme support of the next context API, Apollo support, preact support etc.). Also, it can be problematic when we need to access a child property, like a value.
• A third option would be to support React elements & render props. For instance:

<ToggleButtonGroup
  value={alignment}
  exclusive
  onChange={this.handleAlignment}
>
  {childProps => (
    <Tooltip value="left" title="left" placement="top">
      <ToggleButton {...childProps}>
        <FormatAlignLeftIcon />
      </ToggleButton>
    </Tooltip>
  )}
  <ToggleButton value="left">
    <FormatAlignLeftIcon />
  </ToggleButton>
</ToggleButtonGroup>

It's solving the wrapping issue quite elegantly. However, it doesn't solve the virtualization problem. Notice that I had to move the value property on the Tooltip.

What do you think? I like option 3.

---

I have been benchmark a large number of competitive tooltip implementations. Most of them don't forward the properties. For consistency with all our components, I think that it's best that we forward the properties to the children, it's what will be the first node the DOM structure. This way, our component is more flexible. I'm gonna work on it.

[eps1lon]

I'm not a big fan of the context API either since it's so verbose. But it makes composing the controlled children with other components much easier. Moving the value property up looks really bad. It would be really nice if people could just wrap whatever they want around the controlled child without worrying that they overwrite something important by accident.

It's really tempting to "just do it"™, see what problems we encounter and then benchmark against master.

react-bootstrap is also using cloneElement for ToggleButtonGroup.

[oliviertassinari]

@eps1lon How do you think we should solve the problem?

[eps1lon]

Maybe there isn't even a problem. Concerning this issue it might be sufficient to wrap the icon inside the ToggleButton in the Tooltip.

Maybe it's only a perf issue. But this would require a benchmark against another implementation.

The easiest thing is probably to look into other libraries and check out how they solved the problem and why.

When I watched https://www.youtube.com/watch?v=AiJ8tRRH0f8 I immediately thought about mui and I think there is a great solution in there.

It's just not an immediate problem in my opinion and I'm really unsure which way to go first.

[casank25]

@eps1lon By wrapping icon inside ToggleButton instead of wrapping ToggleButton itself, wouldn't that present an inconsistency?

Based on docs, we wrap Button in Tooltip and we wrap IconButton in Tooltip.

Also, what if ToggleButton needs bigger padding? The user would have to hover over icon in order to see the tooltip.

[eps1lon]

@casank25 Oh I completely agree. This was just a meant as a temporary workaround.

[melissachang]

Actually, if I put title directly on ToggleButton, things work. No Tooltip needed. I updated https://codesandbox.io/s/38w8xry8rq

[JCQuintas]

Couldn't we solve this with hooks now?

[eps1lon]

Couldn't we solve this with hooks now?

Hooks would just improve DX ergonomics. What actually solves this is the context API.

[JCQuintas]

Hooks would just improve DX ergonomics. What actually solves this is the context API.

Sorry, I meant to answer this the problem bellow. I should have been more clear.

I'm not a big fan of the context API either since it's so verbose. But it makes composing the controlled children with other components much easier.

The hook API has an useContext which makes the ContextAPI a bit less verbose for the consumer, although it stays the same for the provider.

[ghost]

Just FYI. As per closed issue #17079, I am unable to wrap my toggle buttons and get full functionality. Clicking the button does fire the event, but the background does not latch to the new toggle state. See https://codesandbox.io/s/create-react-app-jf5qe

I see that it is the same root design issue as described here, in that ToggleButtonGroup requires its buttons to be immediate children. Thanks.

[volodymyr-kushnir]

@leantide you'd just have to set value on your wrapping component and not in your wrapped component, i.e.

<ToggleButton value={"toggle1"}>Toggle 1</ToggleButton>
<Toggle2 value={"toggle2"} />
<ToggleButton value={"toggle3"}>Toggle 3</ToggleButton>
<Toggle4 value={"toggle4"} />

https://codesandbox.io/s/create-react-app-kuhkl
UPD: I assume you have fixed your issue by now, but I commented anyway, because I just had the same issue and thought that perhaps somebody else would also find this thread later. Your codesandbox really helped, thanks.

[xstable]

Simplest answer to me was to add the propery "value" to Tooltip:

  function generateToggleButton(filter, brand, tooltip) {
    return (
      <Tooltip
        arrow
        key={filter}
        title={tooltip}
        value={filter}
        disableHoverListener={tooltip === `brands.${filter}.tooltip`}
        disableTouchListener={tooltip === `brands.${filter}.tooltip`}
      >
        <ToggleButton color="primary" value={filter}>
          {brand}
        </ToggleButton>
      </Tooltip>
    );
  }
     <ToggleButtonGroup
          size="small"
          value={brands}
          onClick={e => setBrandsInQuery(e.target?.value)}
          color="primary"
        >
          {Object.values(ProductBrands).map(
            brand => generateToggleButton(brand, t(`brands.${brand}.label`), t(`brands.${brand}.tooltip`)),
          )}
        </ToggleButtonGroup>

[totszwai]

Guys, Tooltip breaks ToggleButton.

The selected CSS class will not get set if you wraps the Tooltip around the button.

<ToggleButtonGroup>
  <Tooltip title="whatever1">
     <ToggleButton value="blah1">Blah1</ToggleButton>
  </Tooltip>
  <Tooltip title="whatever2">
     <ToggleButton value="blah2">Blah2</ToggleButton>
  </Tooltip>
</ToogleButtonGroup>

Without Tooltip you can see that the proper class is being added:

When wrapping the ToggleButton with a tooltip, it breaks:

[jmca]

Simplest answer to me was to add the propery "value" to Tooltip:

  function generateToggleButton(filter, brand, tooltip) {
    return (
      <Tooltip
        arrow
        key={filter}
        title={tooltip}
        value={filter}
        disableHoverListener={tooltip === `brands.${filter}.tooltip`}
        disableTouchListener={tooltip === `brands.${filter}.tooltip`}
      >
        <ToggleButton color="primary" value={filter}>
          {brand}
        </ToggleButton>
      </Tooltip>
    );
  }
     <ToggleButtonGroup
          size="small"
          value={brands}
          onClick={e => setBrandsInQuery(e.target?.value)}
          color="primary"
        >
          {Object.values(ProductBrands).map(
            brand => generateToggleButton(brand, t(`brands.${brand}.label`), t(`brands.${brand}.tooltip`)),
          )}
        </ToggleButtonGroup>

Passing value also works for me so far, thanks for the tip. Extending TooltipProps interface keeps it valid TS too:

import '@material-ui/core';

declare module '@material-ui/core' {
  export interface TooltipProps {
    value?: any;
  }
}

I still need to test out the solution at #21026 (comment) which is way less code.