inuitcss / CONTRIBUTING

[DEPRECATED] [go to intuitcss/inuitcss]
https://github.com/inuitcss/inuitcss
11 stars 0 forks source link

Consider Abstracting Configuration Settings Checks #8

Closed Undistraction closed 8 years ago

Undistraction commented 10 years ago

There is a repeated pattern throughout Inuit:

1 – Do something if a variable is true

$inuit-enable-list-inline--delimited:   false !default;
@if ($inuit-enable-list-inline--delimited == true) {
}

2 – Warn if a setting is false (This only occurs once, but as the project grows ...)

$inuit-responsive-settings: false !default;

@if ($inuit-responsive-settings == false) {
    @warn "Oops! Have you included a responsive settings file?"
}

It might be nice to abstract this into a couple of simple mixins that would reduce the repetition and make the intent clearer:

1 – Do something if a variable is true

@mixin include-for($setting) {
   @if ($setting == true) {
        @content;
    }
}

@include include-for($inuit-enable-list-inline--delimited) {
}

2 –Warn if a setting is false


@mixin warn-for-missing($setting) {
   @if($setting == false) {
      @warn "Oops! Have you included a #{$setting} file?"
   }
}

@include warn-for-missing($inuit-responsive-settings){
}

Typed this straight in so probably errors.

csswizardry commented 10 years ago

I don’t think this will actually provide any advantages over what we have currently, will it? It would take time to implement a mixin that will only provide functionality we already have…?

Apologies if I’m missing something :)

Undistraction commented 10 years ago

I think it provides big advantages:

  1. Reveals the intention much more clearly.
  2. Moves the decision making process to a single central place
  3. Removes duplication of messages
  4. Removes duplication of boolean equality check

The mixins would be very simple and therefore take a minimal amount of time to implement. Any time spent writing them would offset all the time spent maintaining duplication across all classes that use this same pattern and the unnecessary visual noise it creates. This is framework code, so this level of abstraction is fine I think. It helps get the framework out of the way of the code the framework provides.

Undistraction commented 10 years ago

In fact, you could simplify it further by using a map rather than lots of boolean variables. This would completely remove the need to declare $inuit-enable-list-inline--delimited: false !default; or similar at the top of every file. You could just register the settings file (writing a key to a map), them check for the existence of the key inside include-for/warn-for-missing. Again this removes framework cruft from the files, decreasing noise.

I'd be happy to put this together as a demo if you are at all interested. No bother if not.

csshugs commented 8 years ago

@Undistraction Thanks for pointing this out. You are absolutely right and your solution would improve the framework. But as @csswizardry pointed out, it would take a lot of time (very much modules are affected by this) to implement. Since we are only fixing bugs for this (pre-alpha) version of inuitcss, we will not make the effort of implementing this, unfortunately.

We also don't have something like this in the beta version anymore:

$inuit-enable-list-inline--delimited:   false !default;

@if ($inuit-enable-list-inline--delimited == true) {
    ...
}

...so we don't need such a solution anymore, anyway.

But again, thank you very much for your proposal!