[css] Add the 'css-scope' to all the component CSS styles

DerrikMilligan commented 4 months ago

Currently we have the lovely css-scope property which we can override as documented here.

If this is set with a 2 level, or deeper, specificity such as $css-scope:'.w-app .app' then we get a good chunk of the CSS (currently everything in the scss files) prefixed with this scope. Which is fantastic.

However, there are some instances where because of this it makes certain styles MORE specific than the styles for the components. One example is for the w-table component, with the pagination specifically, the buttons get the size--lg class added to them.

The resulting class with scope for size--lg ends up being .w-app .app .size--lg which is MORE specific than the specificity from the w-table component for those buttons and their font size which is .w-table__pagination .w-pagination__page.

This commit adds the css-scope prefix/wrapper to all the component styles which fixes this issue and keeps the stylings far more consistent across the board.

If you use the hide whitespace option when viewing the Files changed tab you can see that there really aren't that many changes overall. A lot of indentation.

To be fair my use case with Wave may be a little unconventional, where I'm slowly migrating a legacy PHP codebase into using Vue components. Because the w-app gets added to the <body> tag I've had to change the css-scope so that none of the styles/classes affect the original pages. This prevents any leaking which is amazing! But then I have this issue of specificity on some of the components.

antoniandre commented 4 months ago

Mmm @DerrikMilligan. 🤔 This on one hand looks obviously consistent and logical and what I should have done from the beginning and it seems I just forgot. On another hand this is a very big impact that I'm not sure I'm catching the whole reach, this can cause a lot of problems to all the users. Also just to be in sync, what I initially planned is the following:

each component is extremely easy to override with just .w-tabs {} for instance for the simplest case (and this could still work if the SCSS scope was empty)
global helpers classes must have priority over components. E.g. mt0 should override any top margin set in the component definition. As well as a size--lg overrides a w-alert(sm) - not sure if it would now, but at least in the concept definition :)
we don't want to have definitions with !important (like some other frameworks do) E.g. mt0 could have an important so it would always override a more specific rule. But that would be the beginning of the troubles.

I'm not refusing this PR, because it's smart, but I open the discussion to be sure we're considering all the aspects :)

By the way, thanks again for all your work, this is amazing to see someone who is so committed to make this library better and understands so well the codebase and offers quality code in the same style. 🚀 I was thinking it would make sense that we get in touch, and discuss the future plans and components and more! I'd love that you reach me to me email in the package.json :)

DerrikMilligan commented 4 months ago

Hey @antoniandre!

I recognize that this is a large change. And unfortunately I only have the 1 project I'm using Wave in to check to see if there are breaking changes because of it. In theory this follows the pattern that you've started with the CSS scope, which was one of the most appealing things about this framework when I was shopping around for one, and I believe that having looked through the documentation site locally with this change that everything seemed in order.

I also love that you don't use !important anywhere. I hate seeing that and having to fight styles that think they know best. Wave has been easy enough to make CSS tweaks too where needed to match designs.

I agree that the utility classes should override the base styles. But I also think that we should be able to probably handle the css-scope being different since it's something discussed and offered, and that is applied in the majority of places.

But ineed it's a large change and not one to be made lightly. I also recognize that my project is somewhat unconventional. So maybe my use cases shouldn't drive this project as a whole. But it is somewhat an inconsistency currently.

antoniandre commented 3 months ago

Hey @DerrikMilligan! Thanks for taking a stab at this :) After we've merged in, I've tried a lot of different approaches to make this move possible without being a breaking change. The only valid way I found, was something like this:

A mixin that takes a selector that can be empty, and styles to scope if needed.

@mixin nest-if-any($css-selector: '', $styles: '') {
  @if $css-selector != '' {
    #{$css-selector} {
      @each $selector, $properties in $styles {
        #{$selector} {
          @each $property, $value in $properties {
            #{$property}: $value;
          }
        }
      }
    }
  } @else {
    @each $selector, $properties in $styles {
      #{selector} {
        @each $property, $value in $properties {
          #{$property}: $value;
        }
      }
    }
  }
}

But the inconvenience is that the only way to use it is with SCSS maps like follows:

@include nest-if-any('', (
  '*': (
    color: red,
    font-size: 16px
  ),
  'p': (
    color: blue,
    font-size: 14px
  )
));

This makes the css very hard to read and maintain IMO.

So we're left with 3 choices:

We do the change with default scope-selector '*' (the lowest priority) and we don't say anything and release it a minor version. -> This is really not cool for the users because that will insidiously break outside overrides silently.
We release the change with default scope-selector '*' in a MAJOR version. -> users will be warned, and forced to get information how to upgrade and why it's a breaking change. That could also be disappointing and annoying to have a breaking change every now and then for so little added value.
We don't change anything. After all adding a scope makes sense to things that could potentially collide with other styles, but a class prefixed by .w- is unlikely to be styled by external sources. And if ever it would then it would be better if the .w- selector at stake would be the least priority as possible.

I'm thinking to revert this PR and instead find a way to have less specificity when using the size props (xs, sm, ...) if that's the original issue.

Because the w-app gets added to the tag I've had to change the css-scope so that none of the styles/classes affect the original pages. This prevents any leaking which is amazing! But then I have this issue of specificity on some of the components.

-> Have you had a chance to try to mount both your Vue app and Wave UI on #app?

DerrikMilligan commented 3 months ago

Yeah I too was trying to look at some solutions but couldn't find anything that looked better. I agree that the mixin would work but is cumbersome.

I also agree that option 1 and 2 feel bad changing to using *

Option 3 would still just leave the .w-app as the default selector right?

I haven't dug into how I'm mounting on the page yet. But I will hopefully get back to you soon.

antoniandre commented 3 months ago

Yes option 3 is leaving the default .w-app in the $css-scope, but not adding this scope to the components that start with .w-. I meant to have a single class selector when possible for easier overrides. Like .w-input {}.

Just leaving a note here for the future that I'm definitely happy to add a $css-scope with a default empty value in front of each component as long as it can be added as one single wrapper, not impacting the readability/maintainability/ease of writing. For now we haven't found a way to do that and we will leave the components as is. No impact for users, and still easiest to override. :)

antoniandre / wave-ui

[css] Add the 'css-scope' to all the component CSS styles #153