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

Feature/css var deep #428

Closed
wants to merge 10 commits into from
Closed

Conversation

tonyjwalt
Copy link
Contributor

Issue #, if available: #398

Description of changes:
I've added the basics of a CSS variables deep formatter for conversation.

This one also currently supports CSS variables flat, split out dark tokens, and string data-types (CSS has some stings that need quotes, and others that can't have them). I use the split dark tokens in tandem with a custom transform in my current project.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@tonyjwalt
Copy link
Contributor Author

I just wanted to check in on this one. I know it now shows a conflict on the test snap file, but I can fix that easily. Mainly, I want to see if this formatter is going in the direction you'd like. This was approved to have me start via issue #398.

@avkvirtru
Copy link

@tonyjwalt thanks for starting this. I'm brand new here, but tried building your lib/common/formats/css-variables.js locally and couldn't get it working as intended. Some issues observed with the current css/variables-deep format:

  1. not seeing CSS var(--aliases) as expected:
{
  "vds": {
    "line-height": {
      "md": { 
        "value": "1.50"
      },
      "body": { 
        "value": "{vds.line-height.md.value}"
      }
    }
  }
}

produces:

:root {
	--vds-line-height-md: 1.50;
	--vds-line-height-body: 1.50;
}
  1. Comment fields seem to be ignored.

  2. Autogenerated header at the top of the build file is also missing:

/**
 * Do not edit directly
 * Generated on Thu, 06 Aug 2020 18:40:29 GMT
 */

@avkvirtru
Copy link

avkvirtru commented Aug 7, 2020

@tonyjwalt I've hacked together my local version that fixes some syntax (soft tab vs \t, missing a trailing ;) to match the vanilla css/variables formatter.

It even works with nested variables! Meaning this JSON:

{
  "vds": {
    "font": {
      "scale": {
        "base": { 
          "value": "1.6rem", 
          "comment": "assumes font-size 62.5% on html"
        },
        "multiplier": {
          "value": "1.25",
          "comment": "Major Third (4:5) modular scale https://www.modularscale.com/?16&px&1.25"
        }
      },

      "size": {
        "md": {
          "value": "{vds.font.scale.base.value}",
          "comment": "16/24px !!! consider --vds-font-* to set all font properties, not just font-size"
        },
        "sm": {
          "value": "calc({vds.font.size.md.value} / {vds.font.scale.multiplier.value})",
          "comment": "13/18px !!! consider --vds-font-* to set all font properties, not just font-size"
        }
      }
    }
  }
}

Produces this CSS:

:root {
  --vds-font-scale-base: 1.6rem;
  --vds-font-scale-multiplier: 1.25;
  --vds-font-size-md: var(--vds-font-scale-base);
  --vds-font-size-sm: calc(var(--vds-font-size-md) / var(--vds-font-scale-multiplier));
}

I'm really missing the file header and comment support though… @dbanksdesign any thoughts on how to reuse these two helpers in a custom format?

@avkvirtru
Copy link

I'm thinking that perhaps what we really need here isn't a custom format (e.g. css/variables-deep) but a custom transform to be piggy-backed on an existing format (e.g. css/variables)…

This PR loses some of the nice-to-haves of the CSS format and transform group, while duplicating other logic like kebab-case.

@tonyjwalt
Copy link
Contributor Author

This code was really just posted to start the process of comment. If the idea and direction felt right I intended to finish building it out here (which would include the header and comments).

That said, I do have a much more robust version fleshed out in my local project. I chose a custom format because i pipe SCSS-flat, CSS-flat, CSS-deep, SCSS-deep, and versions that split out dark tokens through this. I like having all of those formats able to run through the same logic and checks (like applying quotes to strings when necessary).

I'm happy to throw a few more hours into this over the next week to get it updated and cleaned up further if that makes sense to people.

@avkvirtru
Copy link

@tonyjwalt I haven't had enough time to fully understand or articulate my needs. But for posterity, here are my tweaks to your lib/common/formats/css-variables.js with // AVK version comments for differences… https://gist.github.com/avkvirtru/ab6cb37d36de05ed00790c3856432642

I'm currently playing with a custom transform.

@tonyjwalt
Copy link
Contributor Author

Ok, I've read through your comments and looked at the code as it is on my machine right now. I'm style-dictionary as an npm link against one of my projects to verify everything is working as expected.

A few thoughts. I chose a formatter because I wanted a CSS variables format that maintains variable hierarchy. That means my desire is specific to a given format and determines I use a custom format. If you just want to maintain aliases, I'd recommend a transform at the end of your transform group that just applies the original value of a prop back to the value for the prop.

I think the CSS formatter (and SCSS one) should be looked at and possibly combined. They should go from a template to something a little smarter. A perfect example of why is that some strings need quotes, and others don't. This formatter file is a stripped down version of the formatter I have in my project. I also like being able to use the formater to generate 2 tokens like primary-color and primary-color-dark from a single declaration of primary-color { value: #232323, dark:#434343 } when combined with a simple transform. This makes converting the token to an iOS file much easier.

Here are a few thoughts/edits based on the file you linked me to. Line number on the left refers to the line in code at the link you sent:
general - added the prop.comment to the 'light token'
6 - good catch, i missed that
13,14,20 - made the same adjustment here
33,34 - i agree with no need on .deep for this, however, if we want to use this formatter for other purposes, then it'd be needed (this is what I do in my local project)
67-69 - I like my regex because it's stricter, but am open to discussion. Valid aliases seem to always include references to other token values that are defined by {token.value}. Are you thinking of using references to items that are not a valid token value?
93 - I drop that .value to reference the generated token (not keep the alias)
96 - I swap the . for - because that is what happens when a value goes from {primary.color.red.value} to a token of primary-color-red
99 - this is a CSS variable formatter, so I'm making CSS variables. This allows real-time adjustments of CSS variables that cascade through the styles, and helps with theming things in the shadow DOM.

@avkvirtru
Copy link

@tonyjwalt thanks for taking a look!

67-69. Totally open to stricter regex for tightly scoped changes. I simplified it at some point to get it working and forgot to try your stricter version at the end.

I think for lines 93, 96, and 99, I wasn't asking "why this code" as much as explaining our code via comments :)

I'm hoping to contribute a PR with a small CSS variable transform, because that's all I need to solve my immediate problem. Maybe that will help you with your custom format?

@tonyjwalt
Copy link
Contributor Author

Got it. Thanks for going through the formatter and for your comments and suggestions!

@avkvirtru
Copy link

avkvirtru commented Aug 10, 2020 via email

@mrtnbroder
Copy link

I think it would be great as well if we could choose the selector (:root) in which these variables will be written in. In my case for example, I don't want my variables to live inside :root {} but .ds {}.

@dbanksdesign
Copy link
Member

Circling back to this PR... I believe we have solved this issue with the outputReferences feature coming up in 3.0: https://amzn.github.io/style-dictionary/#/version_3?id=output-references You can use it on a few existing formats and in custom formats to output references rather than have style dictionary resolve them. You can use it on the css/variables format specifically:

 {
  source: [`tokens/**/*.json`],
  platforms: {
    css: {
      transformGroup: `css`,
      files: [{
        destination: `variables.css`
        format: `css/variables`,
        options: {
          outputReferences: true
        }
      }]
    }
  }
}

Here is an example style dictionary setup that uses outputReferences too: https://github.com/amzn/style-dictionary/tree/3.0/examples/advanced/variables-in-outputs

@tonyjwalt if this solves your issue we can close this PR. If there are other things you would like to see let me know and we can try to build those in. Thanks!

@mrtnbroder
Copy link

@dbanksdesign really nice! One thing I found though:

It looks like it can't reference "nested" or "concatened" values (however you want to call it).

E.g.:

global:
  static:
    breakpoint:
      xs:
        value: "304px"
      sm:
        value: "768px"
      md:
        value: "calc({global.dimension.static.breakpoint.xs} / {global.dimension.static.breakpoint.sm})"

with outputReferences: true outputs:

:root {
  --global-dimension-static-breakpoint-sm: 768px;
  --global-dimension-static-breakpoint-xs: 304px;
  --global-static-breakpoint-md: var(--global-dimension-static-breakpoint-sm);
}

but should be:

:root {
  --global-dimension-static-breakpoint-sm: 768px;
  --global-dimension-static-breakpoint-xs: 304px;
  --global-static-breakpoint-md: calc(var(--global-dimension-static-breakpoint-xs) / var(--global-dimension-static-breakpoint-sm));
}

@dbanksdesign
Copy link
Member

@mrtnbroder ah that makes sense. Let me see if I can try to build that in

@tonyjwalt
Copy link
Contributor Author

The outputReferences achieves the same outcome as this PR. It's good to close.

@tonyjwalt tonyjwalt closed this Apr 3, 2021
@dbanksdesign
Copy link
Member

@mrtnbroder this now works (#590) in the latest release candidate version of style dictionary!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants