Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ansgarsteinkamp opened this issue May 18, 2023 · 15 comments
Closed

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

ansgarsteinkamp opened this issue May 18, 2023 · 15 comments

Comments

@ansgarsteinkamp
Copy link

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.

@ansgarsteinkamp ansgarsteinkamp changed the title [BUG] [BUG] classNames are ignored in v5.13.0 May 18, 2023
@danielbarion
Copy link
Member

danielbarion commented May 18, 2023

Hi, I tested here and the issue is:

  • CSS is being injected into the page

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
Copy link
Member

#1027 suggests a workaround

@danielbarion
Copy link
Member

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
Copy link

Tomekmularczyk commented May 19, 2023

Hey guys, I don't think that's problem with the current version. Tailwind classes were ignored with [email protected]. 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
Copy link
Author

ansgarsteinkamp commented May 20, 2023

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
Copy link

@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
Copy link
Member

gabrieljablonski commented Jun 14, 2023

@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
Copy link

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
Copy link
Member

gabrieljablonski commented Jun 14, 2023

@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
Copy link
Member

danielbarion commented Jun 14, 2023

@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:

  • we just disabled the extraction of the CSS into our production build and the rollup is handling the injection of the styles by adding the following code at the beginning of the generated files:
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
Copy link
Member

hey @NemeZZiZZ @gabrieljablonski,

I need help here: #1041

:)

@danielbarion
Copy link
Member

@NemeZZiZZ please check: #1041 (comment)

@NemeZZiZZ
Copy link

@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
Copy link
Member

danielbarion commented Jun 27, 2023

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
Copy link
Author

ansgarsteinkamp commented Jun 28, 2023

Edit: The solution of Tomekmularczyk is working perfectly!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants