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.96k stars 32.27k forks source link

[Tooltip] - add new prop: `childrenTitle` #44378

Open yairEO opened 4 days ago

yairEO commented 4 days ago

Summary

It is helpful for the Tooltip component to optionally be able to use its children as the title.

I suggest a dedicated prop named childrenTitle (or maybe childrenAsTitle) which will be used if title prop is not provided/undefined/null.

Sometimes its not easy to use the same children also for the title attribute because it requires considerable code changes.

Imagine a component which returns some 10-20 lines of JSX. Now I want to wrap everything the component returns with a tooltip, because the element which rendered this component is truncating the content and I want the tooltip to show it as-it-is.

Such a scenario requires creating an extra component just so the title and the children could call it both, which can consume some unwanted effort.

Examples

No response

Motivation

No response

Search keywords: Tooltip, title, children

siriwatknp commented 3 days ago

Is this practical in terms of accessibility? Do you have any resource that we can look into?

yairEO commented 3 days ago

@siriwatknp what do you mean accessibility? what resources? this suggestion is based on common-sense :)

Here's an example:

Assume the below <Foo/> component is heavily used in an app, lets say in table cells, and most or all places where this component is used are truncating their content, hence its likely <Foo/> cannot be fully-viewed by users.

Now we want to add a tooltip so every instance which implements <Foo/> automatically will have a fix for the above UX problem.

Now, assume we want to wrap everything within Foo with a Tooltip, we face a difficulty as to how should we re-structure the code so the content remains and also acts as the tooltip's title prop.

Most non-senior developers will struggle, waist time and most likely end up with some sort of an abomination where code is duplicated or just badly written.

const Foo = () => (
  <div>
    <h1 className="text-2xl font-bold">Main Title which can be very long</h1>
    <p className="text-gray-600">This is the subtitle text which can also be quite long</p>
  </div>
);

export default Foo;

Ideally, a single Boolean prop indicating to the Tooltip component to use its children as the title would save a lot of time:

const Foo = () => (
  <Tooltip childrenTitle>
    <div>
      <h1 className="text-2xl font-bold">Main Title which can be very long</h1>
      <p className="text-gray-600">This is the subtitle text which can also be quite long</p>
    </div>
  </Tooltip>
);

export default Foo;

Even better would be to have this the default behavior if no title prop exists at all:

<Tooltip>
   ...
</Tooltip>

This requires eliminating scenarios where the title is used but have an undefined or null value, which is a clear indication of the wish of the developers not to render a tooltip

siriwatknp commented 3 days ago

siriwatknp what do you mean accessibility?

I meant the screen reader.

Also, based on your example, it does not make much sense to me. You'd use a Tooltip when you want to displays information about the element but if your content is already showing those content, the Tooltip sounds excessive to me.

The title is required to be able to pass the correct aria-label and aria-labelledby to the children.

siriwatknp commented 3 days ago

May be an opinion from Base UI team would confirm about your issue cc @michaldudak @mj12albert

yairEO commented 3 days ago

but if your content is already showing those content, the Tooltip sounds excessive to me.

As I've mentioned in my previous message, Foo's content is not fully visible because it depends where does is rendered in.

In this scenario, its rendered within table cells which truncates the content, hence <Foo/> is not fully visible.

Also, this is an extremely easy feature to implement. I assume writing tests & documentation for it takes double than actually implementing it 😉

michaldudak commented 3 days ago

If I understand the problem correctly, we have a similar issue in Base UI: https://github.com/mui/base-ui/issues/523, but with no ETA at the moment.

siriwatknp commented 2 days ago

As I've mentioned in my previous message, Foo's content is not fully visible because it depends where does is rendered in.

In this scenario, its rendered within table cells which truncates the content, hence <Foo/> is not fully visible.

I see.

yairEO commented 2 days ago

@michaldudak - its not similar at all... I only gave an example of one scenario out of who knows how many.. BTW, I already created a variant Tooltip that perfectly achieves the aim of https://github.com/mui/base-ui/issues/523. But this is really not related to what I am suggesting here.

My suggestion is extremely simple and easy to implement. a no-brainer.

michaldudak commented 17 hours ago

Tooltips are often placed on interactive elements, such as icon buttons. Having the prop you're proposing enabled in such cases doesn't make sense. Since your case is so simple to implement, I'd suggest implementing it in a wrapper component in your application code instead of adding it to the library.

yairEO commented 15 hours ago

@michaldudak - a wrapper sounds logical to me.

tooltips are used in SaaS products for complex content rather than simply wrapping "icons and buttons".

That my be their "traditional" use-cases for simple websites and low-level work, but for those who build complex systems, tooltips are everywhere and can be anything. Can get VERY complex in all sorts of ways.

I get it that most people don't get to work on complex things and are using probably using MUI as-is for their websites and simple needs, and the few who does need more complexity can just create wrappers around MUI and enhance them, and only if that's not easily possible, suggest direct change to MUI team. That is a good philosophy which I can understand and agree with. Makes sense.