ConsenSysMesh / rimble-ui

React components that implement Rimble's Design System.
https://rimble.consensys.design/
MIT License
462 stars 67 forks source link

Icon display prop not honored #380

Closed MikeLockz closed 4 years ago

MikeLockz commented 4 years ago

Describe the bug When Tooltip wraps an Icon, the Tooltip does not show attached to Icon

To Reproduce Steps to reproduce the behavior:

  1. Wrap Tooltip around Icon
  2. Hover Icon
  3. Tooltip is not attached to Icon

https://codesandbox.io/embed/tooltip-not-attached-to-icon-erzjo

Expected behavior Tooltip should appear next to Icon

Screenshots If applicable, add screenshots to help explain your problem.

image

Rimble UI Version

gesquinca commented 4 years ago

@MikeLockz The example for this bug shows an button and no icons and the tooltip appears to be working correctly.

MikeLockz commented 4 years ago

@gesquinca - Updated the failing example. Also happening in the demo app in the rimble-ui repo.

gesquinca commented 4 years ago

@MikeLockz I'm still only seeing a Button and a Tooltip that seems to be attaching fine on this link. https://codesandbox.io/embed/tooltip-not-attached-to-icon-erzjo

Anyhow, the tooltip is placed relatively to the element it wraps. If the element spans the length of the screen, like something with display: block it will attach to that. The icons are set to display block so the tooltip is attaching correctly.

MikeLockz commented 4 years ago

Figured out that I can't click the link and edit those sandboxes either. I need to login first 🤦‍♂️ Figured that out and updated the examples.

https://codesandbox.io/s/tooltip-not-attached-to-icon-erzjo?from-embed

It's happening to our live docs too so I think it's worth figuring out a solution for this that doesn't show a floating tooltip on the Icon.

https://rimble.consensys.design/components/rimble-ui/Tooltip#examples

I didn't realize that Icon has its display defaulted to 'block'. I tried to add a prop value like display="inline" but am still getting a full-width Icon component. The only way I am able to get the Icon to behave inline is by wrapping it in a Box component first and setting the Box to display="inline" and then wrapping the Box by the Tooltip.

Checked the source and I think wrapping the Icon in the div is causing the display prop to not create an inline element out of the Icon. I don't think this is an issue with the Tooltip, but with Icon. How would we create an inline Icon, other than wrapping in a Box or using Flex? If the display prop has no effect, we shouldn't offer it as a prop to the component. I was able to get around this like:

const StyledBox = styled(Box)``;

const Icon = React.forwardRef((props, ref) => (
  <StyledBox display={props.display} ref={ref}>
    <RmdIcon.default {...props} />
  </StyledBox>
));

I think this is where Icon was at one point. Not sure why it changed to a div from a Box as a wrapper around RmdIcon.default.