adaptlearning / adapt-contrib-vanilla

The core bundled adapt theme
GNU General Public License v3.0
11 stars 64 forks source link

inconsistent styling using bg-color mixin #304

Open chucklorenz opened 3 years ago

chucklorenz commented 3 years ago

Using AT 0.10.5 with FW 5.15.2

I like having built-in classes and mixins. Unfortunately, I feel the style inconsistencies are counter-intuitive enough to count as a bug. Or am I misunderstanding??

The image below uses Vanilla's built-in .bg-color-mixin: @acme-slate: #2f4454; @acme-rosefont: #da7b93; .bg-color-mixin(acme-slate, acme-rosefont);

It was applied in the AT in page Classes.

image

Comments in theme-extras.less says it can be applied to course, contentObject, article, block, or component. No mention is made of style cascade. I think it is counter-intuitive to expect that applying a "bg-color" would expect a cascade. Applying a bg-color to a single article or block is an act of making an exception to the theme's general styles. I would expect that I am modifying that single element.

And the cascade is inconsistent:

  1. The blue seen in the menu/page title and article title is Vanilla's unaffected style.
    • Since the mixin was applied to the page's classes, one would expect that the title color would reflect the mixin's font color (rose).
    • I would not expect anything in the article to be affected by bg-color added to the page classes. But if the background changes, it would be consistent for the title to change--but it doesn't. (FYI: this is not a matter of a transparent article background showing the pages background. Per inspecting the CSS.)
  2. The first block had a different bg-color applied to the block's Classes. It should be a grey background with black text. Inspector shows both fonts colors competing for the block title with the same level of specificity. But the page's rose color wins out for some reason--compile order, perhaps??
  3. The second/final block has nothing declared in its classes. I would expect that it would be white background with a teal blue title and charcoal body font color. I'd expect the same for its components.
  4. Because I'm modifying a block background, I have no expectation that it would be changing the component's background and fonts. I don't think it should--but this is the only case that I think an argument can be made for the cascade.

Also the bg-color applied to the page awkwardly pokes through the margin on the menu item: image

On the other hand, the header-color mixin which is intended for use with page headers is much much more consistent (since the rule is less expansive). The menu item, too, is unaffected.

image

chucklorenz commented 2 years ago

Using AT 0.10.5 with FW 5.15.2 with Vanilla 5.7.0

guywillis commented 2 years ago

Hi @chucklorenz,

Thank you for raising this as an issue and apologies for the rather tardy response on my part.

The move from block-color to bg-color was to try and reduce the specificity of the mixin so it could be applied to multiple levels - course / content object / article / block / component - but, and this is a failure on my part, it's not written explicitly enough that it may not work at each of these levels without further development work or that it will cascade down.

The mixin itself predominately targets the block and component levels and could relatively swiftly be scaled up to also target articles. In fact, by 'limiting' the mixins application to these three levels (article / block / component) it should hopefully resolve the inconsistent application. I use the word limit here not in the physical sense of restricting the mixin but by removing reference that it can be applied at higher levels.

Addressing your bullet list:

  1. The page header is a bit of an anomaly and always has been in Adapt. As you mention, the header-color` mixin explicitly targets the header section and does not impact anything else.
  2. This issue, as you rightly pointed out, is due to the compile order.
  3. I would expect children to inherit their parents state. If a page or article had a blue background using bg-color, I would expect all blocks within to inherit this blue colour. If a stricter application is required I would expect the bg-color mixin to be moved down a level of specificity and applied at the block level instead.
  4. To aid the ease of background colour application, and to avoid potential text contrast issues, header text areas inherit the inverted colour variable with this cascading down levels.

In summary, I would suggest making the following amendments:

I'm keen to know your thoughts on the above and look forward to your response.

chucklorenz commented 2 years ago

Hi @guywillis

Extra classes and mixins are a bonus. Because these mixins are "extras," I believe the Vanilla author (you in this case) can make them work in any gosh-darn way he/she wants--as comprehensive or as granular. But I think as you do, that comments alongside the mixins ought to reflect its intended use. So ultimately I'm OK with any [or no] modification you make to the mixin code so long as the comments better reflect the mixin's intended use. An addition that I think might be helpful: in the comments identify the role of the mixin parameters: one determines the background color, one determines the font color. Might help the understanding of someone unpracticed in reading Less mixins (my hand is raised!!).

Yes, I'd like to see a version of the mixin that is applied to an article and that cascades to its child blocks. I think it would be better still if it incorporated the page, too, and cascaded to all of its child containers. (Exclude the page header, I think that needs a mixin that targets it separately. So the only thing on the page level that gets modified is the background; but applying it to the page is meaningful because the cascade impacts all of its articles.)

But I'd want this cascading mixin as an alternative to, not as a replacement for, targeted mixins. Can we have both? I'd still want a mixin that targets the container I applied it to and that does not cascade to its children. In other words, I'd like a mixin that changes an article's background and font color without affecting its blocks. We have a targeted mixin for the block. We'd need one for the article. I guess you could do one for the page if you felt the need for completeness, but there are other easy ways to assign a background color to a page. We have a targeted mixin for the page header, too. Perhaps one for a page footer (doesn't matter if it is not in Adapt repos)--again if you feel the need for completeness. And let's ignore the component because the standard, unmodified Adapt component is simply not constructed to take accept a background. (No padding, etc.) Is one helpful for menu items? Perhaps, but low priority for me.

If we have any mixins that cascade, I think it will be important to examine the compile order. If .bg-color-mixin can be applied to both an article and to a block, someone will apply it to both in the same page. It is reasonable for them to expect that the mixin that they applied to the block will override the cascade from the mixin applied to the article.

Guy, here's my recommendation to you:

  1. Rework .bg-color-mixin so that it works with articles and blocks and so that it cascades. (Think about this mixin as the DRY cascade-through-everything mixin.)
  2. Rework the comments introducing .bg-color-mixin
  3. Create .article-color-mixin to parallel .block-color-mixin and construct it so that it does not cascade through to its blocks.
  4. Un-deprecate .block-color-mixin
  5. As time allows, try to extend .bg-color-mixin so that it cascades through "all" containers.
  6. As time allows, try to add other targeted mixins a la .block-color-mixin to other [worthy] containers.

Hope this is helpful. And happy to continue the conversation here or in PM if desired.

chucklorenz commented 2 years ago
  1. While working on this issue, I have found specificity to be a crucial issue. All these mixin styles complete with styles originating with plugin or Vanilla. Unfortunately, it seems that some plugins apply styles to directly to the element, e.g., title, while others apply them to the inner div, e.g., title-inner. Increased specificity seems to be a more robust strategy than relying on compile order.

  2. If bg-color-mixin were to be a "global" strategy--apply it on any CO-A-B-C container and it styles all its children--is there a user expectation that it can be applied in more than one place? and is it expected that bg-color used on a block will override a different bg-color used on the page? I think this would be a reasonable expectation for anyone working with HTML/CSS/Less.

So far I have been unable to create such a global mixin, one whose styles are overwritten by usage on a child.

We could achieve increased specificity (and success) is we introduced another parameter representing the element: .bg-color-mixin(@element, @color, @color-inverted) used as bg-color(block, black) But this greatly multiplies the number of declarations needed in advance of its use. So much so that it seems that it would be more effective (less code and less user prep) to create separate mixins, one for each CO-A-B-C container.

Thoughts?

oliverfoster commented 1 year ago

@guywillis any thoughts?