twbs / bootstrap

The most popular HTML, CSS, and JavaScript framework for developing responsive, mobile first projects on the web.
https://getbootstrap.com
MIT License
170.84k stars 78.87k forks source link

Generate %placeholder utilities to @extend from #33861

Open donquixote opened 3 years ago

donquixote commented 3 years ago

Motivation: !important in utiliites

My colleague likes to use SASS @extend with bootstrap utility classes, e.g.

header {
  @extend .py-1;
}

I personally tend to disagree, but I guess it is a valid technique.

One problem when doing this is we also inherit the !important part, which is reasonable on actual utility classes, but not in most of the @extend use cases.

Proposed solution: %placeholder.

Generate placeholder utilities in addition to the regular utilities, using SASS placeholder syntax. https://sass-lang.com/documentation/style-rules/placeholder-selectors They could then be extended like so:

header {
  @extend %py-1;
}

This would have the same effect, but without the !important.

Implementation

In the generate-utility(..) mixin it would look like this:

@mixin generate-utility(..) {
  [..]
      // Generate the real utility.
      .#{$property-class + $infix + $property-class-modifier} {
        @each $property in $properties {
          #{$property}: $value if($enable-important-utilities, !important, null);
        }
      }
      // Generate placeholder utility.
      %#{$property-class + $infix + $property-class-modifier} {
        @each $property in $properties {
          #{$property}: $value;
        }
      }

I applied this as a quick hack, and it actually worked! I was able to @extend the generated placeholder utilities.

Userland workaround?

I was going to do this in custom code, but this would require to copy the entire generate-utility() mixin.

donquixote commented 3 years ago

Userland workaround: u() mixin.

For anybody interested, here is a mixin that can be used in a similar way as @extend .utility-class.

Usage

header {
  // Using @extend - this will add "!important".
  @extend .py-2;
  // Using the mixin - this will not add "!important".
  @include u(py, 2);
  // Using explicit values instead of named values:
  @include uv(py, 20px);
  // Using explicit CSS:
  padding-top: 20px;
  padding-bottom: 20px;
}

Mixin code

$utilities-by-class: ();

@each $key, $utility in $utilities {
  $properties: map-get($utility, property);
  // Use custom class if present
  $property-class: if(map-has-key($utility, class), map-get($utility, class), nth($properties, 1));
  $property-class: if($property-class == null, "", $property-class);
  $utilities-by-class: map-merge($utilities-by-class, (
    $property-class: $utility,
  ));
}

@mixin u($class, $value-name, $breakpoint: null) {

  // Extract values from utility.
  $utility: map-get($utilities-by-class, $class);
  $values: map-get($utility, values);
  $value: map-get($values, $value-name);

  @include uv($class, $value, $breakpoint);
}

// Mixin to use instead of extending utility classes.
@mixin uv($class, $value, $breakpoint: null) {

  // Extract values from utility.
  $utility: map-get($utilities-by-class, $class);
  $properties: map-get($utility, property);

  // Regular (responsive) CSS.
  @include media-breakpoint-up($breakpoint) {
    @each $property in $properties {
      #{$property}: $value;
    }
  }

  // RFS rescaling.
  @if type-of($utility) == "map"
    and map-get($utility, rfs)
    and ($breakpoint == null or map-get($grid-breakpoints, $breakpoint) < $rfs-breakpoint)
    and (map-get($utility, responsive)
      or breakpoint-min($breakpoint, $breakpoints) == null)
  {
    @media (min-width: $rfs-mq-value) {
      @each $property in $properties {
        #{$property}: $value;
      }
    }
  }
}
mdo commented 3 years ago

@donquixote Hell yeah, that is an awesome set of mixins. Want to open a PR?

donquixote commented 3 years ago

@donquixote Hell yeah, that is an awesome set of mixins. Want to open a PR?

Sure why not! Any suggestions where to put it, how to name things etc? Especially where to put the variables processing. to fill the $utilities-by-class variable.

donquixote commented 3 years ago

@mdo ^ (sorry forgot to tag you)

mdo commented 3 years ago

I think https://github.com/twbs/bootstrap/blob/main/scss/utilities/_api.scss is the place to start for those mixins. Haven't tested that placement myself yet, so id be curious if that works or if we could do earlier in the import stack (like in the mixins file).

ffoodd commented 3 years ago

A few considerations with this suggestion:

  1. regarding naming, u and uv are too cryptic: something like get-utility or apply-utility (you get the idea) is preferable,
  2. setting custom value doesn't make sense, simply write your property-value pair. I'd be in favor or disabling it totally by checking that value is in existing values already. Would allow to drop a mixin and stick with one.
  3. I think the provided example might be simplified by using our existing map-get-multiple() function.

IMHO the first suggestion (generating utilities placeholders alongside utilities classes) is simpler to implement and more robust, being inside our utilities API. Mixin could fail for many reason (invalid value, etc.). Moreover placeholders might only be generated when $enable-important-utilities is set to true.

donquixote commented 3 years ago

regarding naming, u and uv are too cryptic: something like get-utility or apply-utility (you get the idea) is preferable,

I don't have a strong opinion, but perhaps others do. I leave some time for bikeshedding :)

setting custom value doesn't make sense, simply write your property-value pair.

The uv() can be preferable to explicit CSS for "abstract" property combos like padding px,py, ps, pe, and the same for margin mx, my, ms, me. Also the responsive parameter. Although for this I might prefer an explicit media query block.

I'd be in favor or disabling it totally by checking that value is in existing values already. Would allow to drop a mixin and stick with one.

Not sure what you mean. Do you want a single mixin with a "flexible" signature, where the parameter can either be a key for a known value, or an explicit custom value? I usually don't like flexible signatures :)

I think the provided example might be simplified by using our existing map-get-multiple() function.

So we would drop the $utilities-by-class variable, and instead do a lookup each time? The motivation for $utilities-by-class was performance, I want to do this only once. On the other hand, if the mixins are only used a few times, then it will be faster to do the lookup each time, because then we don't need to process all the variables.

IMHO the first suggestion (generating utilities placeholders alongside utilities classes) is simpler to implement and more robust, being inside our utilities API.

I am undecided about this. Using @extend with utilities has two problems: 1. It adds !important where you don't want it. 2. It produces ugly CSS, with increased file size, if the selectors are longer than the property-value pairs. Originally when I opened this issue, I only wanted to tackle the first problem. Now I see that the mixin could fix both. One benefit of @extend is that you immediately see how you could refactor your html to achieve the same effect.

Mixin could fail for many reason (invalid value, etc.).

If somebody calls the mixin with an invalid value, SASS will produce an error and they need to fix it. I don't see how this is a blocker. Or do you mean something else?

Moreover placeholders might only be generated when $enable-important-utilities is set to true.

I would make this a separate setting. You should be allowed to disable the !important without breaking all your @extend statements.

ffoodd commented 3 years ago

I'm not talking about flexible signatures: IMHO a single mixin is enough and should bail early if $value does not match any values for the $key utility name.

I still don't think custom value makes sense, to me custom CSS should not be abstracted that way and benefits from being explicit. That's only an opinion here :D

Regarding map-get-multiple, I think something's overdone in your first draft. Don't have time right now to suggest specific improvements, but using map-get-mutliple instead of your custom loop could help, and adding a function like map-deep-get() to avoid successive map-get() could, too.

The placeholder idea was precisely to drop the !important, wasn't it? So it fixes the main pain point. Producing ugly CSS is a matter of opinion, but I'd say we provide tool and people use it: writing bad/good or ugly/beautiful code doesn't matter here. The set of mixin you"re now prodiving will also bloat final file size if not used sparingly, I don't see the difference.

A Sass error needs to be handled, so outputting a warning or error as useful as possible to help users is needed. That's the point. And consequently, you can be sure that any code resulting in an error will end up as an issue here, with us trying to help and understand. That's something to take into account, too.

I really like the idea but that's not an easy feature to deliver. Really worth it though, so still thinking about it :)

donquixote commented 3 years ago

Actually my first draft above is broken, because it does not support the case where the same short key is used in multiple utilities (e.g. margins and negative margins). I already have a different version in my own project:

$utilities-by-class: ();

@each $key, $utility in $utilities {
  $properties: map-get($utility, property);
  // Use custom class if present
  $property-class: if(map-has-key($utility, class), map-get($utility, class), nth($properties, 1));
  $property-class: if($property-class == null, "", $property-class);
  $group: map-get($utilities-by-class, $property-class);
  @if ($group) {
    $group: map-merge($group, ($key: $utility));
  }
  @else {
    $group: ($key: $utility);
  }
  $utilities-by-class: map-merge($utilities-by-class, ($property-class: $group));
}

@mixin u($class, $value-name, $breakpoint: null) {
  @debug 'u()', $class, $value-name, $breakpoint;
  // Find utilities that match the shortcut key.
  $group: map-get($utilities-by-class, $class);
  @each $key, $utility in $group {
    $values: map-get($utility, values);
    // If the values are a list or string, convert it into a map
    @if type-of($values) == "string" or type-of(nth($values, 1)) != "list" {
      $values: zip($values, $values);
    }
    // $values could be a map or a list. @each covers both.
    @each $k, $value in $values {
      @if $k == $value-name {
        @include _utility($utility, $value, $breakpoint);
      }
    }
  }
}

// Mixin to use instead of extending utility classes.
@mixin uv($class, $value, $breakpoint: null) {
  @debug 'uv()', $class, $value, $breakpoint;
  // Find utilities that match the shortcut key.
  $group: map-get($utilities-by-class, $class);
  // Use a trick because SASS does not have 'break' in loops.
  // See https://github.com/sass/sass/issues/378.
  $firstgroup: true;
  @each $key, $utility in $group {
    @if ($firstgroup) {
      @include _utility($utility, $value, $breakpoint);
    }
    $firstgroup: false;
  }
}

// Mixin to use instead of extending utility classes.
@mixin _utility($utility, $value, $breakpoint: null) {

  $properties: map-get($utility, property);

  // Regular (responsive) CSS.
  @include media-breakpoint-up($breakpoint) {
    @each $property in $properties {
      @debug '_utility', $property, $value, $breakpoint;
      #{$property}: $value;
    }
  }

  // RFS rescaling.
  // @todo This might not work at all.
  @if type-of($utility) == "map"
    and map-get($utility, rfs)
    and ($breakpoint == null or map-get($grid-breakpoints, $breakpoint) < $rfs-breakpoint)
    and (map-get($utility, responsive)
      or breakpoint-min($breakpoint, $breakpoints) == null)
  {
    @media (min-width: $rfs-mq-value) {
      @each $property in $properties {
        #{$property}: $value;
      }
    }
  }
}

This does not address your points, I m just posting it here because it works better than the other version.

ffoodd commented 3 years ago

Thanks for sharing!

donquixote commented 3 years ago

Regarding map-get-multiple, I think something's overdone in your first draft. Don't have time right now to suggest specific improvements, but using map-get-mutliple instead of your custom loop could help, and adding a function like map-deep-get() to avoid successive map-get() could, too.

I need to take time to try this.

The placeholder idea was precisely to drop the !important, wasn't it?

My original motivation was that I saw my colleague spamming the @extend with utility classes. I had a bad feeling about it, and then tried to identify why I don't like it. The !important is the most obvious problem, but there are others. I found some interesting blog post when searching for "sass avoid @extend", or "sass avoid @extend for utility classes". https://jdsteinbach.com/sass/use-extend/ https://www.sitepoint.com/avoid-sass-extend/ https://www.smashingmagazine.com/2015/05/extending-in-sass-without-mess/ https://csswizardry.com/2014/11/when-to-use-extend-when-to-use-a-mixin/

One conclusion from this is to use @extend with large rulesets and simple selectors, but use mixins for short rulesets, and especially when you have unreasonably nested selectors (which you should avoid, but this is a different story). Utility classes, by definition, have short rulesets, and the places in our project where I saw @extend being used had deeply nested selectors. The resulting CSS would have long selector lists, followed by a simple CSS rule like padding: .5rem;. In browser developer tools with CSS maps enabled, the rule would point to the @generate-utility() mixin, not the place of the @extend statement.

The set of mixin you"re now prodiving will also bloat final file size if not used sparingly, I don't see the difference.

The file size is caused by repetition. We can either repeat the selector, or repeat the ruleset (or do it properly to avoid repetition - different story). If you have long rulesets and short selectors, then @extend will generally produce smaller file size. If you have short rulesets and long selectors, then mixins will produce smaller file size. Try it!

A Sass error needs to be handled, so outputting a warning or error as useful as possible to help users is needed.

Good point, will do that when I made progress on other points.

ffoodd commented 3 years ago

That's a completely different story.

We won't solve your issue since it's basically people not knowing Sass and CSS enough.

Utilities are not meant to be extended but applied in the markup. Most of utilities values are available as variables too, like $spacers. If one wants to apply a spacer, the way is to use map-get($spacers, "1") or similar.

Considering this, our discussion feels like we're talking about a way to consume our utility API internally instead of simple outputting classes.

Turns out placeholder are a way, tour mixin too, but IMHO the best way is still to write CSS sparingly.

Still thinking out mois 😄

donquixote commented 3 years ago

We won't solve your issue since it's basically people not knowing Sass and CSS enough.

The background is we are working with a system where traditionally you would just accept whatever html is being produced by 3rd party software, but we are gradually taking control of the html so that we can use bootstrap and utility classes the way they are meant to be used.

Utilities are not meant to be extended but applied in the markup.

I generally agree - although I would like to have more arguments to put this into words.

This said, having @include or @extend statements that mimic the utility classes that we would apply in the markup seems like a good preparation, that will make it easy to refactor the html later. It allows to already think in utility classes, even at a time when we don't have full control of the html.

Also, as mentioned earlier, I do like the abstractions of "start" and "end" vs "left" and "right" that provide free RTL support, and also the combinations of "vertical" = top + bottom and "horizontal" = left + right = start + end.

That's a completely different story. We won't solve your issue since it's basically people not knowing Sass and CSS enough.

Now I wonder what has changed since my original post, and what you are willing to support or not support. The essence of my "back story" was already revealed in my initial post.

Still thinking out mois  "mois"?

donquixote commented 3 years ago

This said, having @include or @extend statements that mimic the utility classes that we would apply in the markup seems like a good preparation, that will make it easy to refactor the html later. It allows to already think in utility classes, even at a time when we don't have full control of the html.

Also it makes it easier to "translate" between a style guide where we use utility classes all the way and a part of the application where we might have to accept the given markup and and classes.

ffoodd commented 3 years ago

"mois" was for loud, sorry for the wrong spelling, my french keyboard sometimes wakes up and tries to fix things.

Nothing changed, I still support the idea and think it could be pretty useful in some context. It could even be used in our core to ensure consistency, at some point.

But we need to think about valid use cases to implement this in the most valuable way. As I said (or tried to say 😆), still thinking out loud about this.

mdo commented 3 years ago

The set of mixin you"re now prodiving will also bloat final file size if not used sparingly, I don't see the difference.

The file size is caused by repetition.

From my perspective, we will never use the utilities in our own source Sass.

Utilities are not meant to be extended but applied in the markup. Most of utilities values are available as variables too, like $spacers. If one wants to apply a spacer, the way is to use map-get($spacers, "1") or similar.

Considering this, our discussion feels like we're talking about a way to consume our utility API internally instead of simple outputting classes.

Building on the first quoted comment here, this hits the same way for me. This isn't for us. This is for people who want utility, but without the HTML pollution. This is what Tailwind does with @apply—building components out of atomic property-value pairs. This way you're still using the approach and methodology of Bootstrap, but you're still building components like us.


I think there's a very valid use case here of consuming utilities in a way that meets developers and designers where they are. Some are in the markup, some are still wanting to write component CSS without all the frills. Instead of creating your own $component-* variables that reassign existing Bootstrap values, you could use the utilities that are built on our own system wide rules (margin, padding, text, font, etc are all great examples of this).

<div class="d-flex align-items-center justify-content-center p-3 mb-4 bg-light rounded-3">...</div>

Or a hybrid approach of something like this...

.custom-component {
  display: flex;
  align-items: center
  justify-content: center;
  @include utl(p-3);
  @include utl(mb-4);
  @include utl(bg-light);
  @include utl(rounded-3);
}
ffoodd commented 3 years ago

I get the point, the only concern I have now is about accepting any value: I think the mixin should only accept values already declared in utilities.

Moreover, thinking another way: shouldn't it work as @include utility('pb-md-2'); for example, to really match generated utilities?

mdo commented 3 years ago

I get the point, the only concern I have now is about accepting any value: I think the mixin should only accept values already declared in utilities.

I agree with that.

Moreover, thinking another way: shouldn't it work as @include utility('pb-md-2'); for example, to really match generated utilities?

Keeping it succinct would be nice, but yes also aligning to the utilities themselves is nice. Definitely a balancing act. Not sure if it's better to stack the responsive padding or to encourage media queries?

// Option 1
.selector {
  @include utility('p-2');
  @include utility('p-md-4');
}

// Option 2
.selector {
  @include utility('p-2');

  @media (min-width: 768px) {
    @include utility('p-md-4');
  }
}

I feel like option 2 scales better to combining with custom styles. Like if you add in the flex styles above with these, which feels better to you?

donquixote commented 3 years ago

From my perspective, we will never use the utilities in our own source Sass.

I think there's a very valid use case here of consuming utilities in a way that meets developers and designers where they are.

+1

This is what Tailwind does with @apply—building components out of atomic property-value pairs.

I only had a very superficial look at tailwind. I found this page, https://tailwindcss.com/docs/extracting-components. It assume @apply is not really SASS, but rather something magical happening in client side js?

I get the point, the only concern I have now is about accepting any value: I think the mixin should only accept values already declared in utilities.

I still think some utilities can be useful with custom values. Or alternatively, we could have mixins like @include mx(1rem) for margins, paddings, left/right/top/bottom.

Moreover, thinking another way: shouldn't it work as @include utility('pb-md-2'); for example, to really match generated utilities?

Ok for me. This means we need a different variable to hold those utility names.

  @media (min-width: 768px) {
    @include utility('p-md-4');
  }

Redundancy department of r... If you already have the media query, you want to omit the -md- part. Also, I think you would use a mixin for the media query, to be sure to use the predefined break points.

@include media-breakpoint-up(md) {
  @include utility('p-4');
}

Doing it like this will group all the media query stuff together, and avoid repeating the media query. On the other hand, @include utl(p-md-4) might look more familiar for people who otherwise like to work with those utility classes.

donquixote commented 3 years ago

For a PR: I need one file to prepare the variable(s), and one file to declare the mixin. Or it could be the same file. And we should agree on utl vs utility. Even nicer would be just @utility, but I don't think this is possible in SASS - or is it?

You might ask why am I still talking, instead of producing a PR? Simple answer, I want a PR that has a good chance to be accepted :)

donquixote commented 3 years ago

Please don't jump on the PR just yet, I will run some tests first and might fix some problems that I find.

donquixote commented 3 years ago

The PR #34102 seems to work!

Now it is time to iron out the details:

Optional features:

donquixote commented 3 years ago

Btw there are some tricks here that we could also use to simplify and optimize the generate-utility() mixin:

donquixote commented 3 years ago

Another thing: I see this a lot:

@if type-of($utility) == "map"

Am I correct in my understanding that utility entries where type != "map" are completely ignored?

We could precompute a filtered utilities map that does not include those ignored entries, and where everything is normalized:

$utilities-prepared: (
  "-mx": (
    "-0": (
      responsive: true,
      properties: (margin-left, margin-right),
      value: 0,
    ),
    "-1": (
      responsive: true,
      properties: (margin-left, margin-right),
      value: 0.25rem,
    ),
  ),
  "-shadow": (
    "": (
      responsive: false,
      properties: (box-shadow),
      value: $box-shadow,
    ),
  ),
  "": (
    "-invisible": (
      responsive: false,
      properties: (visibility),
      value: hidden,
    ),
  ),
)
mdo commented 3 years ago

Am I correct in my understanding that utility entries where type != "map" are completely ignored?

At least one check is to confirm that the utility itself hasn't been removed via false as a replacement for the map of options.

  • Option to mark utilities as "virtual" so that the classes are not generated, but you can still use them with the mixin.

This would be cool. Would like to see this in a subsequent update.

  • Currently it is supported in the PR, but it will lead to duplicated media queries if you include multiple responsive utilities with the same break point.

For this reason, I think we should avoid the responsive variants, unless we add another tool to consolidate media queries, which might be weird and unexpected for folks


Answering some other questions:

Is it possible to import the mixin in the _mixins.scss stylesheet as well, or does the functionality require specific placement?

mdo commented 2 years ago

Picking this up in https://github.com/twbs/bootstrap/pull/35465, hoping you have some thoughts to weigh in with too, @donquixote! :D

gastlich commented 2 years ago

Thanks everyone for your contribution. This feature looks really good 👍

I just wanted to add a few bits to this topic after doing some research.


I was recently investigating a potential usage of Tailwind CSS in my project. The thing, which caught my attention was exactly what you have been debating here: a set of predefined rules (mixins), which can be reused by their @apply functionality.

I develop mostly server-side rendered websites (python, django), thus it's quite difficult to easily "link" all of the amazing libraries developed in node.js ecosystem. I try to make it up by using css preprocessors and creating my own custom css classes to prevent overbloating HTML code.

It would be amazing to see a UI library, which follows what Tailwind CSS did, but without all of this magic applied on the top.


Currently, bootstrap is about to start exposing utility mixins, which is going to make this possible:

.custom-component {
    display: flex;
    align-items: center
    justify-content: center;
    @include utl(p-3);
    @include utl(mb-4);
    @include utl(bg-light);
    @include utl(rounded-3);
}

That's, nice for now, but what would be even more interesting to see in the future is to be able to turn this:

<div class="card card-block">
    <h5 class="card-title h3 text-success">Special title treatment</h5>
</div>

into:

<div class="special">
    <h5 class="special-title">Special title treatment</h5>
</div>
.special {
    // @extend .card;
    // @extend .card-block;
    // or
    // @extend %card;
    // @extend %card-block;
    // or
    // @include card;
    // @include card-block;

    &-title {
        // @extend .card-title;
        // or
        // @extend %card-title;
        // or
        // @include card-title;

        @include util(h3);
        @include util(text-success);
    }
}

Thanks to this approach I end up having a set of custom cards with a dedicated purpose. Additionally, I minimize coupling between UI framework and html code. It also makes it easier to upgrade UI library with breaking changes.

Of course we are all aware how @extend "works" in Sass, thus extending css classes solution doesn't work at the moment.

I would consider doing everything on mixins to stay consistent with util mixin, but maybe it can be implemented as component mixin?

Another potential solution could be extending %placeholders. One of the benefits is that it's possible to define them with sass variables:

// Loop over each utility property
@each $key, $value in $spacers {
  .p-#{$key}, %p-#{$key} {
      padding: $value;
    }
}

which generates both utility class name and a placeholder to be extended. The problem with them is once again limitation of @extend and not being able to extend classes outside of the current media scope.

There are some workarounds, but the most interesting which I found was: https://www.sitepoint.com/cross-media-query-extend-sass/

TLDR: generating a placeholder for every breakpoint and picking the right one while using breakpoint mixin.


All this said I believe there is room for a framework, which offers something what Tailwind CSS did, but in less-magical format. I think a framework apart from exposing the final API should also give tools to recreate the API, which in this case are css classes.