material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[material-ui][SpeedDialAction] Props id does not appear in HTML

Open MatetlotDev opened this issue 2 years ago • 6 comments

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/proud-water-fyxu9l?file=/src/App.js

Reproduce

  1. Launch the sandbox
  2. Open up the inspector
  3. Try to search The Ids of the SpeedDialAction components

Actual result

The IDs are not present in the html button components.

Expected result

See the IDs of the actions on the button components.

Current behavior 😯

We do not see the Ids on the button elements

Expected behavior 🤔

We should see the Ids

Context 🔦

I want to add some IDs on the actions so I can make some tests and query the actions by their IDs

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

MatetlotDev avatar Feb 16 '23 11:02 MatetlotDev

Thanks for the report. The SpeedDialAction follows the rules we have for forwarding extra props - it spreads them on the root element. However, in this case, the root element is a tooltip, and having these extra props on it is confusing and likely doesn't make sense.

We can't change how it works currently without introducing a breaking change, but fortunately, there is a workaround you can use: the FabProps prop.

{actions.map((action) => (
  <SpeedDialAction
    key={action.name}
    icon={action.icon}
    tooltipTitle={action.name}
    FabProps={{ id: action.id }}
  />
))}

It sets its props on the button itself, so this should help you target the button.

I'll add this to the v6 milestone, so we can consider redesigning the component in the next major version.

michaldudak avatar Feb 16 '23 14:02 michaldudak

Alright, I understand now why I couldn't see my IDs, thank you very much for your workaround, It does work perfectly indeed !

MatetlotDev avatar Feb 16 '23 15:02 MatetlotDev

Let's leave the issue open, so we remember about this problem when designing the next version.

michaldudak avatar Feb 16 '23 16:02 michaldudak

optional

On Thu, Feb 16, 2023, 5:24 PM Michał Dudak @.***> wrote:

Let's leave the issue open, so we remember about this problem when designing the next version.

— Reply to this email directly, view it on GitHub https://github.com/mui/material-ui/issues/36220#issuecomment-1433356136, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2FFCYLPH4M5KM3VNUEQHS3WXZIEVANCNFSM6AAAAAAU6AXP5A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Wire1lovet avatar Feb 16 '23 16:02 Wire1lovet

I see pros and cons with following the rule of forwarding extra props on the root element in SpeedDialAction. While it's consistent with every other component, it's counterintuitive for devs who don't know how it's implemented (the root element is a Tooltip). At the same time, we have FabProps today, so the issue can be circumvented. Note that the plan to deprecate FabProps is on halt (see https://github.com/mui/material-ui/issues/41281).

@mnajdova @DiegoAndai, do you know if we have other components that break this rule? Or are there other components that have the same "problem" where the root element is not what it seems to be?

aarongarciah avatar Jul 17 '24 12:07 aarongarciah

After some discussions with the team, we decided to change the approach and we'll be forwarding props to the SpeedDialAction button instead of the tooltip. This is what devs expect. We'll also add a slot for the tooltip in case someone needs to customize the tooltip. We'll also take a look to other components to see if some of them would benefit from such a change.

aarongarciah avatar Jul 22 '24 08:07 aarongarciah

We can make the change in stages.

  1. Add slots/slotProps where the root would be the Fab and the tooltip will be a different slot
  2. Update the docs to teach people to use this API
  3. Deprecate the FabProps
  4. In the next major, make the root props being spread on the Fab.

mnajdova avatar Aug 19 '24 10:08 mnajdova