ReactTooltip / react-tooltip

React Tooltip Component
https://react-tooltip.com/
MIT License
3.6k stars 528 forks source link

[BUG] classNames are ignored in v5.13.0 #1025

Closed ansgarsteinkamp closed 1 year ago

ansgarsteinkamp commented 1 year ago

Bug in v5.13.0: classNames like the Tailwind-Class bg-blue-900 are ignored somehow:

<Tooltip className="z-50 bg-blue-900" id="Home" delayShow={200}>
   Home
</Tooltip>

With v5.12.0 everything works just fine.

danielbarion commented 1 year ago

Hi, I tested here and the issue is:

CSS works like objects, when we pass a custom class, this is the behavior:

Before v5.13.0

  1. import CSS styles:
  2. component receive a new class
  3. browser process and render it
.tooltip-default-css {
  background: red;
}

.bg-green-500 {
  background: green;
}

--------- same as:

Object.assign({ background: 'red' }, { background: 'green' })

On version v5.13.0:

  1. component receives a new class
  2. CSS is injected into the page
  3. browser process and render it
.bg-green-500 {
  background: green;
}

.tooltip-default-css {
  background: red;
}
--------- same as:

Object.assign({ background: 'green' }, { background: 'red' })

this is a bug, we will take a look as soon as possible. We are open to suggestions on fixing this because the CSS class from props and injected CSS have the same CSS specificity.

Solution 1 -> you need to increase the CSS specificity from your side - I don't like to force the community using our library to write extra code

Solution 2 -> we need to increase the specificity of the CSS received by props (how?) - I prefer this approach, but I don't have any ideas on how to do that at the moment - edit: as far as I checked, it's not possible, the best solution to fix this bug is solution 1. I'll wait for the community if someone has some suggestions to fix this issue. I really don't want to do a rollback of the feature to inject style by default instead of importing CSS.

reference: https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity

gabrieljablonski commented 1 year ago

1027 suggests a workaround

danielbarion commented 1 year ago

yeah, that workaround is just the dev increasing the specificity of his CSS query, it's the solution 1 that I mentioned.

another way to do that together with taiwlind is:

HTML - JSX
<div className="container">
  <Tooltip className="tooltip" />
</div>

CSS
.container .tooltip {
  @apply bg-green-500;
}
Tomekmularczyk commented 1 year ago

Hey guys, I don't think that's problem with the current version. Tailwind classes were ignored with react-tooltip@5.8.3. This is why I had to create separate CSS class like:

.tooltipWrapper { 
  @apply rounded-lgplus bg-tixy-1000 p-2 text-sm font-normal leading-4 text-tixy-10 shadow-sm;
}

or with the current version:

.tooltipWrapper[role="tooltip"] { 
  @apply rounded-lgplus bg-tixy-1000 p-2 text-sm font-normal leading-4 text-tixy-10 shadow-sm;
}
ansgarsteinkamp commented 1 year ago

Edit: Thank you for the solution, Tomekmularczyk, I didn't understand it correctly at first. It's working perfectly!

Tomekmularczyk

There is definitely a problem with the current version 5.13.0. Everything works fine with version 5.12.0.

NemeZZiZZ commented 1 year ago

@danielbarion , how to completely remove default styling of tooltip? We use this lib hardly in our project, but every time I need to find some crazy way to override it's internal stylings :( Why just not add removeStyle prop for those who can deal with it low-level?

gabrieljablonski commented 1 year ago

@NemeZZiZZ There's no equivalent to a removeStyles prop, as it would definitely be a rarely used feature. You can just use the style prop to override whatever you need.

<Tooltip style={myCustomStyle} />
NemeZZiZZ commented 1 year ago

Daniel did it on my request for v4 just before v5. I think it's not so rare, you just have to create some crazy stuff to avoid style overlappings if styles are totally separated of code in your project and looks completely different. style doesn't make you happy because you have no access to entire tooltip's markup and states this way. I said this last year, will write it again:left and top calculations are more than enough to make tooltip work if you have completely different stylings

gabrieljablonski commented 1 year ago

@NemeZZiZZ

That's totally understandable, but by our experience this hasn't been an issue to the package users in general.

You're welcome to open a PR with a rough idea of how you suggest we do this (doesn't have to be perfect, just enough to get the idea) so we can better discuss it.

danielbarion commented 1 year ago

@NemeZZiZZ, as I said before, we are trying to not roll back the decision to inject the styles by default, but when reading your comments, I started to think about refactoring the inject the styles function.

Today:

function styleInject(css, ref) {
  if ( ref === void 0 ) ref = {};
  var insertAt = ref.insertAt;

  if (!css || typeof document === 'undefined') { return; }

  var head = document.head || document.getElementsByTagName('head')[0];
  var style = document.createElement('style');
  style.type = 'text/css';

  if (insertAt === 'top') {
    if (head.firstChild) {
      head.insertBefore(style, head.firstChild);
    } else {
      head.appendChild(style);
    }
  } else {
    head.appendChild(style);
  }

  if (style.styleSheet) {
    style.styleSheet.cssText = css;
  } else {
    style.appendChild(document.createTextNode(css));
  }
}

The refactored code:

  1. enable code extraction to prevent rollup injecting the styles by default
  2. move this style injection function to our lib and add a prop to disable it if needed

This change will also give us more control, like the style injection inside of web components as example.

I don't promise, but I'll try to take a look at this as soon as possible.

danielbarion commented 1 year ago

hey @NemeZZiZZ @gabrieljablonski,

I need help here: https://github.com/ReactTooltip/react-tooltip/pull/1041

:)

danielbarion commented 1 year ago

@NemeZZiZZ please check: https://github.com/ReactTooltip/react-tooltip/pull/1041#issuecomment-1605726779

NemeZZiZZ commented 1 year ago

@NemeZZiZZ please check: #1041 (comment)

I'm sorry for being late with no support. Well, I want to tell, that this just works. Maybe it's not so obvious in daily usage, but styleless and className-based re-styling fully can be achieved at last! That's great, thanks! Surely, it needs to be documented for others - but for now I just wait for next release with no patience :) Thank you so much, Daniel!

danielbarion commented 1 year ago

Available at v5.16.0 - please note to use v5.16.1 ;)

I'll close this issue as resolved, @NemeZZiZZ and @ansgarsteinkamp please feel free to open a new issue if needed,

ansgarsteinkamp commented 1 year ago

Edit: The solution of Tomekmularczyk is working perfectly!

@danielbarion In version 5.16.1, the issue remains unchanged. bg-blue-900 is still ignored.