Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible bug in typography-mixins output in plugin-sass #198

Closed
torstendaeges opened this issue Feb 17, 2024 · 6 comments
Closed

Possible bug in typography-mixins output in plugin-sass #198

torstendaeges opened this issue Feb 17, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@torstendaeges
Copy link

Thank you again for the quick fixes!

I think I found another one: When using plugin-sass in conjunction with plugin-css (CSS Variable Mode), the "default" mode of mixins will use CSS variables, while the following modes are still using the actual values. As I unserstand it, they too should be using the variables to be fully dynamic.

Here's a sample output as generated by the input below:

  "fontalias": (
    default: (
      "font-family": (var(--fontalias-font-family)),
      "font-size": (var(--fontalias-font-size)),
    ),
    "l": (
      "font-family": (Helvetica, -system-ui, sans-serif),
      "font-size": (2rem),
    ),
    "m": (
      "font-family": (Helvetica, -system-ui, sans-serif),
      "font-size": (1.5rem),
    ),
    "s": (
      "font-family": (Helvetica, -system-ui, sans-serif),
      "font-size": (1rem),
    ),
  ),

Here's the tokens.config.mjs ...

import pluginSass from '@cobalt-ui/plugin-sass';
/** @type {import('@cobalt-ui/core').Config} */

export default {
  tokens: ['./tokens.json'],
  plugins: [
    pluginSass({
      filename: 'test.scss',
      pluginCSS: {
        filename: 'test.css',
        modeSelectors: [
          {mode: 'l', selectors: ['[data-viewport="l"]']},
          {mode: 'm', selectors: ['[data-viewport="m"]']},
          {mode: 's', selectors: ['[data-viewport="s"]']},
          ],
      },
    }),
  ],
};

...and the tokens.json to reproduce this:

{
    "bodyTextSmall": {
      "$type": "typography",
      "$value": {
        "fontFamily": ["Helvetica", "-system-ui", "sans-serif"],
        "fontSize": "1rem"
      }
    },
    "bodyTextMedium": {
        "$type": "typography",
        "$value": {
          "fontFamily": ["Helvetica", "-system-ui", "sans-serif"],
          "fontSize": "1.5rem"
        }
      },
      "bodyTextLarge": {
        "$type": "typography",
        "$value": {
          "fontFamily": ["Helvetica", "-system-ui", "sans-serif"],
          "fontSize": "2rem"
        }
      },
    "fontalias": {
        "$type": "typography",
        "$value": "{bodyTextLarge}",
        "$extensions": {
            "mode": {
                "l": "{bodyTextLarge}",
                "m": "{bodyTextMedium}",
                "s": "{bodyTextSmall}"
            }
        }    
    }
}

Hope this isn't the way it is supposed to be. :) Thank you!

@drwpow
Copy link
Collaborator

drwpow commented Feb 17, 2024

Thank you, keep the bug reports coming! I probably won’t be able to get to this till next week if that’s OK.

Normally I’d encourage you (or others) to make a PR but this specifically is a nasty bug that’s probably caused by some of the architecture decisions. Don’t let me stop you from trying if you’re interested! But though the fix is relatively quick, it’s not straightforward at all and slightly-annoying to wade through

@drwpow drwpow added the bug Something isn't working label Feb 17, 2024
@drwpow
Copy link
Collaborator

drwpow commented Mar 3, 2024

Ah so actually, this is generating correctly in plugin-sass as-intended. But it may not be as you expected.

The current behavior of plugin-sass with modes is to always return the raw value rather than the CSS variable. The reason is: with the modeSelectors set up in CSS, those should actually handle modes for you. When using token("fontAlias") in Sass, it will pull the var(--font-alias-*) variables with the understanding that the modeSelectors will dynamically change the value as-needed.

However, if we used variables for token("fontAlias", "m"), then it would return the same value, and thus there wouldn’t really be “mode” selecting, in a sense. The only way to override this would be if we had, say --font-alias-m in :root, we could return that. But since that would be ignoring the modeSelectors, it would essentially work the same way as the raw values would (unless you were applying other overrides, in which case Cobalt won’t know what you’re doing and can’t anticipate it).

So all that to say, perhaps the usage of token("…") in Sass when specifying a mode isn’t intuitive. Or perhaps there’s a usecase I’m missing. In any case, would love feedback if there’s anything that could be improved here.

@torstendaeges
Copy link
Author

We can certainly see where you're coming from regarding the mode-handling in plugin-sass' mixin output. If I understand correctly, we'd normally use the standard "." value of the mixin, which respects css mode switching. So the modes of the mixin are only used if we need to override css mode switching - but unlike how it's done with the "$__token-values" (where, thanks to the bugfix, the mode targets are used), the overrides use raw values.

Using raw values here makes sense to me and seems to work for us, but I keep wondering if that prevents some usecases:

Say there are typography tokens with modes (e.g. l, m, s) and the values of their properties are aliases. Say these aliases point to tokens that again have modes. In such a case, using raw-values instead of the mode targets for the overrides would possibly circumvent additional alias-levels and therefore break those other mode switches. So I guess my solution would be to use raw values if the targets are tokens with raw values and the mode targets as css variables if these targets have aliases as their values.

Does that make sense? Hope I could describe this in an understandable way.

@drwpow
Copy link
Collaborator

drwpow commented Mar 13, 2024

Yes that makes sense! I’ve actually been investigating a related problem with modes and theming in the DTCG format discussions: mode scoping. You won’t find the term “mode scoping” explicitly referred to by any single comment, and it’s just a term I came up with to describe this problem. But if you look carefully, you’ll find this problem exists in nearly every comment, from every team implementing it. I’ve been trying to outline the problem in some notes I’ve been taking on the modes problem:

Another problem is “mode scoping,” an issue that’s been talked about frequently in the DTCG GitHub and elsewhere, but something no proposal addresses. Many design systems implement some variation of base → semantic → component token layers (example). But because there are no restrictions over whether modes cross these “layers” or not, it can lead to confusion and churn.

As a practical example, pretend you have a “base” grayscale from 100 – 1300 (values don’t matter), and you decide your semantic.text-color token is gray.700. This ends up being “light mode,” and when you add “dark mode,” you use gray.600 to get roughly the same contrast. Shortly on, you realize that your dark mode gray.600 needs to be adjusted to improve contrast—you need a color that doesn’t exist in gray.100 – 1300 at all. So the question becomes: does semantic.text-color alias gray.700 for light and dark modes, and the modes are thrust onto gray.700 so it has multiple values? Or does that go against the idea of “stateless” core tokens, so we create a separate scale of gray-light.XXX and gray-dark.XXX, so light and dark modes can exist on the semantic layer)? The former answer is more “clever” but blurs the line between the core and semantic layers as the core takes on more “awareness.” The latter answer preserves the layer boundaries better, but introduces the idea that there are “rules“ over when core tokens can/can’t be used in certain contexts (arguably also blurring the lines, but in a different manner). Either answer will “fork” the design system in 1 of 2 ways that is difficult/impossible to reverse down the line.

A hand-wavy answer to this is “it’s up to every team to decide.” But when we consider every design system implementing modes faces this same confusion, and it stems directly from how teams define modes/theming (i.e. the purpose of this document), this may not be a difference of opinion so much as it a symptom that modes/theming are ill-defined in any token system (and could probably be remedied with a more opinionated solution).


So back to your suggestion:

So I guess my solution would be to use raw values if the targets are tokens with raw values and the mode targets as css variables if these targets have aliases as their values.

That’s how you and your team are using aliases, but other teams may have other assumptions about how aliasing works. And you also have strong opinions in that thread like “core tokens should never have aliases; all aliases should only be at the semantic layer and semantic tokens should only alias core tokens.” Which still doesn’t answer the question of how modes do/don’t apply to aliases, but is just another example.

If you’re confused by now, so am I 😄. My whole point is “this problem has layers to it, and many people have their own assumptions about how it should/shouldn’t work,” and so even some of Cobalt’s current behavior has been tiptoeing around the modes ↔ aliasing problem and shouldn’t be considered intentional decisions (not all).

But I’m currently talking with others involved in DTCG to have more of an opinionated take on this, because I think we are all starting to realize that any attempt at resolving this can happen in the DTCG format itself. And it would save all of us from having to solve these same problems in different ways.

@torstendaeges
Copy link
Author

torstendaeges commented Mar 17, 2024 via email

@drwpow
Copy link
Collaborator

drwpow commented Dec 4, 2024

Was going through issues and I realize I forgot to reply to this one! So sorry! 🙇 Yeah this was a stumper, because this is yet another usecase of “different design systems need different things” (which is understandable! that is the purpose of a design system—to codify the needs of an organization). There’s no right or wrong, but really the core issue of “when do the modes get applied?” has been such a bugbear that it has delayed a lot of conversations about what modes are.

I’ve been working on getting the new Resolver Spec ready for consideration for DTCG acceptance, which should be an upgrade to modes. It takes the basic idea of modes, but codifies a clearer “when/how does this mode take affect?” which even bleeds into the confusion of, say, what gets put into .scss files. Because you can only trigger certain modes to have effect based on the values available to you in that specific output language.

It’s tricky stuff! The direction I’ve taken with Cobalt 2.0/Terrazzo is for the Sass plugin to almost completely merge with the CSS plugin, and it now only prints out CSS variables. Especially when you take into account how powerful and flexible CSS variables are, and how many new killer features are in CSS that extend off these. So the “stock” plugins will probably keep their existing behavior, but the differences in needs are exactly why Terrazzo has a documented plugin system. It should be easy to fork any existing plugin and customize it (and the new version is even easier!). And if you need any help at all with this I’d be glad to pair on it 🙂

@drwpow drwpow closed this as completed Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants