Allow className prop

[emmgfx]

Description

It's a proposition and shouldn't be merged, yet.

This change allows to use className property on HolyLoader component, for example:

<HolyLoader className="bg-red-400" />

What means this? Mainly that it's more customizable, but also that works properly with tailwindcss (which is important to me).

Why it's a breaking change? Because if I use a className like bg-red-400, it's overrided by the style prop background (because of css specificity), so I wrote this:

if (this.settings.className) {
  // Next line includes the class attribute
  this.bar.className = this.settings.className;
  // Next line removes background from style in order to not override the className
  this.bar.style.removeProperty('background');
}

What should be the correct behavior?

Type of change

Please delete options that are not relevant.

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] This change requires a documentation update

Checklist:

• [x] My code follows the style guidelines of this project
• [x] I have performed a self-review of my code
• [x] I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [x] My changes generate no new warnings
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] New and existing unit tests pass with my changes

[tomcru]

This is an interesting idea! I'm currently sick, but will have a look asap ❤️

[emmgfx]

Thanks @tomcru . If you have some idea or think that I could improve something, please let me know and I'll update the PR.

[tomcru]

I've had some time to think about this and I think it would be great if setting any classes in className would also overwrite other properties. Because I would expect it to also work for something like h-[8px] as a user.

This, however, does not work with the current approach - because HolyLoader is styled inline. If we were to switch it to a class-based approach (for example, use .holy-loader for styling), then adding classes via className after the initial .holy-loader class should theoretically overwrite those values, making it work for all properties.

I can test this as soon as possible, otherwise feel free to hack away at it in this PR ✌️

Typed on my phone so it might be a bit messy 🫣

Update: this seems to be a bit tricky because of CSS cascading rules & tailwind. At first look, I could only get it to work with appending ! important to the Tailwind classes, for example: <HolyLoader className="!h-[18px] !bg-[red]" />. I had created a <style> element with the .holy-loader class - which takes precedence over Tailwind, as Tailwind is only loaded as a .css file.

[emmgfx]

Maybe the way to go is to leave HolyLoader with default styles and let it customize only through standard props (className, style).

I mean, remove the boxShadow, height and color properties, and let it be customized by classes like in the Tailwind case className="bg-red-400", or by inline styles with style={{background: "red"}}.

Maybe then it would be nice to expose two components, the current <HolyLoader /> and maybe his children <HolyProgress />. This <HolyProgress /> should be optional and accept the className and style props that i'm talking. The first and always required <HolyLoader /> could also accept some customization properties, imagine that you want to use a background to improve contrast, or even the height should be defined in this required component.

If the default styles are loaded through .css file, this would work?