New: Expand header-color mixin to articles and blocks, add prefixed colors (fix #595)

[swashbuck]

Fix #595

New

• Expand header-color mixin to article and block headers
• Add prefixed color class options to header-color and bg-color mixins

Testing

Add header-color black to an article and block with header text. The background should be black and the text color is white.

[kirsty-hames]

Hey @swashbuck, thanks for the PR. I've tested this and have a few observations:

1. Inherited styles on child headers

Applying the header-color mixin at page or article level cascades styles down to all child headers, not just the targeted model. For example, applying the mixin class to an article causes the child block header to inherit the same styles (see screenshot). The same behaviour is observed when applied at page level. I'm assuming we'd want to tighten the scope to style the direct header only?

2. Completion indicator (PLP) styles missing

Since completion indicators can be applied at both article and block level, the mixin should account for this. Suggest including something like the following:

.is-article,
.is-block {
  .pagelevelprogress {
    &__indicator {
      border-color: @@color-inverted;
    }
    &__indicator-inner {
      background-color: @@color;
    }
    &__indicator-bar {
      background-color: @@color-inverted;
    }
  }
}

PLP not styled by mixin:

PLP styled by mixin:

3. Header padding

Article and block headers currently have no default padding, so the background colour sits flush against the text. The existing .comp-header-bg-color mixin, handles the padding as part of its implementation. Should we follow the same pattern here, or is the expectation that padding will be added via custom theme CSS?

[swashbuck]

Thanks, @kirsty-hames

1. Inherited styles on child headers - Should now be fixed.

2. Completion indicator (PLP) styles missing - Implemented. For simplicity, I'm applying to menu and page as well even though they don't currently allow completion indicators. Might be something that gets added in the future. Otherwise, do you think we should rewrite to only apply to articles and blocks? It would make it slightly more complicated but doable.

3. Header padding - Still thinking about this. We already have block header spacing variables. However, they default to 0 and we probably don't want to have the same padding for block headers with and without background colors. Separately, I was going to propose a new .expand-header class as well that would make the block headers full width.

4. Conflict with bg-color (new concern) - I've realized that we would not be able to use both header-color and bg-color since they both rely on a non-prefixed color class. For instance, we couldn't set the block header background color to black and the block background color to blue due to how the mixins are set up.

header-color black bg-color blue

One solution could be to allow colors to be prefixed so that it's clear which mixin it applies to. This would be backwards compatible since it would only introduce an alternate way to specify a color.

header-color header-black bg-color bg-blue

bg-color mixin

.bg-color-mixin(@color, @color-inverted) {
  .bg-color.@{color},
  .bg-color.bg-@{color} {
    // ... same body as today
  }
}

header-color mixin

.header-color-mixin(@color, @color-inverted) {
  .header-color.@{color},
  .header-color.header-@{color} {
    // ... same body as today
  }
}

Thoughts on this or any other ideas?

[swashbuck]

@kirsty-hames I've implemented issue 4 in ca7d5e0 , but let me know what you think.

[kirsty-hames]

@kirsty-hames I've implemented issue 4 in ca7d5e0 , but let me know what you think.

This works for me thanks @swashbuck. Issue resolved and it still has legacy support for existing courses.