unovue / radix-vue

Vue port of Radix UI Primitives. An open-source UI component library for building high-quality, accessible design systems and web apps.
https://radix-vue.com
MIT License
3.44k stars 213 forks source link

[Feature]: add dsiabeld prop to tooltip #572

Closed peter-emad99 closed 6 months ago

peter-emad99 commented 9 months ago

Describe the feature

it will be good to hava the ability to disable the tooltips based on certain condition.. for now i use big nubmer for the delay option to act like disabled .. also i tried to use the open prop with like this shouldDisable ? false : undefined but it didn't work

Additional information

peter-emad99 commented 9 months ago

for notice it worked with me when it put :key="shouldDisable" (causing rerender) and :open="shouldDisable? false : undefined"

michael-harvey commented 9 months ago

From a UX point of view, wouldn't it be preferable for tooltips to just not be rendered if a condition for their content isn't met?

peter-emad99 commented 9 months ago

i don't get what you mean tottaly .. but here the scenario i wanted the disabled props to exist ... i have collapsed sidebar .. when it is expanded the button shouldn't show the tooltip ( the label of buttons already shown) but when it is collapsed the tooltip should show ... i ended up doing the solution I wrote above :key="shouldDisable" :open="shouldDisable? false : undefined" but it will be straight forward to have disabled prop if it is true it work as normal ..if false it just do nothing

michael-harvey commented 9 months ago

If it's expanded, you could render standard buttons and if it's collapsed, buttons with tooltips. Tooltips still do a bunch of calcs to get the initial positioning of the pop up etc, so it's better to just not render any of that if it's not being used.

jeffpohlmeyer commented 7 months ago

From a UX point of view, wouldn't it be preferable for tooltips to just not be rendered if a condition for their content isn't met?

Here's a situation. I'm displaying content in a window that has a strictly-defined width. I would like to manually truncate this text if the length of it is above a certain character length, in which case I would use a tooltip to display the content, but it is superfluous to display a tooltip if the length of the content is within my constraints. Not argumentative, but what suggestion would you give to me in a situation where I'm more used to the Vuetify implementation?

MellKam commented 6 months ago

@jeffpohlmeyer I don't see the problem in here.

<TooltipRoot v-if="text.length > X">
    <TooltipTrigger>
        {{ text.slice(0, X) }}
    </TooltipTrigger>
    <TooltipContent>
        {{ text.slice(X) }}
    </TooltipContent>
</TooltipRoot> 
<span v-else>{{ X }}</span>
jeffpohlmeyer commented 6 months ago

@MellKam in that very simple situation, yes, but we have places in our app where the tooltip needs to show up on, for example, a dialog trigger or a dropdownmenu trigger, which can complicate things because of how one needs to render them together.

MellKam commented 6 months ago

I fully agree with the @michael-harvey. We don't need a disabled state for the tooltip. This will complicate the logic, and worsen accessibility and performance. If you don't need to show the tooltip, just don't render it.

MellKam commented 6 months ago

@jeffpohlmeyer If you need to truncate the text only in this place, then use this condition only in this place. Don't move logic that you use in only one place to a common ui component

jeffpohlmeyer commented 6 months ago

I must not have made my point clear. The text that shows up in the dialog/dropdownmenu trigger may or may not need to be truncated. The trigger itself may be based on user input.

I suppose one solution could be

<TooltipContent v-if="X.length > 48">
  {{ X }}
</TooltipContent>

But that feels very hacky and may not actually address the issue regarding position calculations referenced in https://github.com/radix-vue/radix-vue/issues/572#issuecomment-1872591352

MellKam commented 6 months ago

@jeffpohlmeyer I wouldn't recommend you to render the TooltipContent component conditionally. If you don't need to see the tooltip, then don't render it at all (The whole tree staring from TooltipRoot).

Can you describe in detail why you think this solution won't fit you? You can create some wrapper component to reuse it in other places

<TooltipRoot v-if="text.length > X">
    <TooltipTrigger>
        {{ text.slice(0, X) }}
    </TooltipTrigger>
    <TooltipContent>
        {{ text.slice(X) }}
    </TooltipContent>
</TooltipRoot> 
<span v-else>{{ X }}</span>
jeffpohlmeyer commented 6 months ago

I'm using Shadcn-Vue so it's not a one-to-one, but this is the situation I'm working with:

<Dialog>
  <TooltipProvider>
    <Tooltip>
      <TooltipTrigger as-child>
        <DialogTrigger as-child>
          <Button variant="ghost">{{ truncateText(X, 36) }}</Button>
        </DialogTrigger>
      </TooltipTrigger>
      <TooltipContent>{{ X }}</TooltipContent>
    </Tooltip>
  </TooltipProvider>
  <DialogContent>
    ...
  </DialogContent>
</Dialog>

with

function truncateText(value: string, truncateLength: number, clamp = '...'): string {
  if (!value || value.length <= truncateLength || !truncateLength) return value
  return `${value.substring(0, truncateLength)}${clamp}`
}

The text that populates the button is user-created, i.e. it is the name of a widget that the user decides, and for the purpose of looking pretty I obviously want to truncate it at 36 characters there. It is also superfluous to have a tooltip render if the length of the text is less than 36 characters (aside from the fact that it's also been told to me by people higher up than me that I should not display a tooltip if the text is not truncated). Based on what you're suggesting, the solution is this:

<Dialog>
  <TooltipProvider v-if="X.length >= 36">
    <Tooltip>
      <TooltipTrigger as-child>
        <DialogTrigger as-child>
          <Button variant="ghost">{{ truncateText(X, 36) }}</Button>
        </DialogTrigger>
      </TooltipTrigger>
      <TooltipContent>{{ X }}</TooltipContent>
    </Tooltip>
  </TooltipProvider>
  <DialogTrigger v-else as-child>
    <Button variant="ghost">{{ truncateText(X, 36) }}</Button>
  </DialogTrigger>
  <DialogContent>
    ...
  </DialogContent>
</Dialog>

which, I can see will work, but it seems counter-productive because then there are two buttons for which changes need to be made if/when said changes are needed. It simply seems to me that it would be much more straightforward to be able to disable the tooltip under these conditions.

MellKam commented 6 months ago

@jeffpohlmeyer You can make some kind of wrapper for that. This is just an example, you can come up with better name and API.

<TooltipProvider>
    <Dialog>
        <TruncationTooltip :text="X" :chars-limit="36">
            <DialogTrigger as-child>
                <Button variant="ghost">{{ truncateText(X, 36) }}</Button>
            </DialogTrigger>
        </TruncationTooltip>
        <DialogContent>
            ...
        </DialogContent>
    </Dialog>
</TooltipProvider>

Overall, I think this is not the problem that library like radix-vue should solve. We just provide primitives for you to build your own ui

jeffpohlmeyer commented 6 months ago

Yeah, almost as soon as I wrote that I thought that I could just create the component myself and add a disabled prop and conditionally render in the manner you initially described. Thanks for helping

zernonia commented 6 months ago

Thanks for the input here guys! After looking through the w3 pattern and issues, I don't think there's any downside in terms of a11y for implmenting this.

When the tooltip is not opened, the TooltipTrigger actually just behave as a button. Only if it is open, TooltipTrigger will then populate aria-describedby.

@Chrtyaka had the PR ready, and WDTY @MellKam ? 😁

MellKam commented 6 months ago

@zernonia Oh, okey then it seems like accessibility is all right. After checking the component source I found that we don't really make any tooltip position calculation until the tooltip is opened. So should not be the big problem for performance. Maybe it does have a sense to add disabled props.