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

feat(format): output references #504

Merged
merged 22 commits into from
Feb 2, 2021
Merged

feat(format): output references #504

merged 22 commits into from
Feb 2, 2021

Conversation

dbanksdesign
Copy link
Member

@dbanksdesign dbanksdesign commented Dec 11, 2020

Issue #, if available: #17 #396

Description of changes: Allow for variable references in output files. These changes will make it easier to use the name of a referenced token rather than the resolved value, so the output will keep the aliases/references intact.

  • Adding resolveReference() and usesReference() methods on the dictionary object that is passed to formats and actions. This will allow custom formats and actions to test and resolve references the same way style dictionary does internally, allowing you to get the transformed name of a token for instance.
  • Adding a outputReferences configuration on SCSS, CSS, and Less variables, as well as ios/class.swift, flutter/class.dart, and android/resources formats to make use of this
  • Adding a 'output-references' example to show how to use these new methods
  • Added documentation about this feature
  • Added integration testing to properly test this

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

@chazzmoney
Copy link
Collaborator

WOOHOO! #17

@chazzmoney
Copy link
Collaborator

chazzmoney commented Dec 11, 2020

I really like that this empowers people on the route of #268, which is something that I have wanted for SD for a LONG time (even well before I created that issue).

I think it is super valuable that people can customize SD for how they think about tokens. It fits our usually conflicting goals: simply to use; fully customizable / working how people want or expect; enable and encourage people to build in a manner that organizes their thinking (use both base/defining properties and use-case/output properties).

This also enables client-side/end-platform tools to have strong access to the underlying reference structure that powers SD which is a huge plus.

Really really excited about this one.

@chazzmoney
Copy link
Collaborator

chazzmoney commented Dec 11, 2020

Three improvements to consider:

  1. I think this deserves documentation catering to the absolute newest / least experienced SD user. This is a very important / useful thing and making sure people know it is available and can understand how to use it easily will help them use SD to its fullest.
  2. Based on the usefulness to downstream tooling, we may want to make this "on" by default... this will be included in a major version/breaking change so it might be ok... we should probably @ some of our most active folks here to get their opinion.
  3. The configuration options here should likely be able to be set at three levels: global, included file, and property (maybe even inheritably so)

@chazzmoney
Copy link
Collaborator

Also... we should probably update all default formats to use variables where appropriate.

It will be a lot of work, but it is weird to include this without updating all of them.

lib/common/formats.js Show resolved Hide resolved
lib/common/formats.js Outdated Show resolved Hide resolved
return;
}

for (i = 0; i < path.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that it's been decided to move away from lodash, and the "utils" folder has started to contain lodash-like functions, do you think we should isolate this as deepFind function there?

@didoo
Copy link
Contributor

didoo commented Dec 16, 2020

First of all, amazing PR @dbanksdesign, really great idea 🙌.

Now, I did a quick code review and added a few comments there. I have some other thoughts here:

  • I am not entirely sure about the name useVariables. maybe there is something that could be more explicit on what it does? I was thinking something like preserve[References/Variables] or use[Variables|References]InOutput
  • agree with @chazzmoney that we should consider also other less-obvious default formats that may support this kind of referencing in output (which one? JS clearly yes, in some form; but what about others like formats for iOS or Android?)
  • regarding setting it on by default for version 3.0, agree as well, makes sense (unless there are downsides we're overlooking)
  • regarding the three levels configuration, not sure about it. why do you think we need this extra layer of complexity in the codebase? if you can achieve the result adding a set of useVariables: true wherever you need it for the output, I don't think is much of a burden for the final user, and in any case, you probably do it once at the initial setup and then forget.
  • regarding the documentation, there are other parts that we may want to highlight for the new user (eg. my colleague didn't realize the input format for the tokens could be directly JavaScript code, not only JSON) so maybe this could be done in a separate task?

Let me know if I can help somehow on this.

@didoo
Copy link
Contributor

didoo commented Jan 14, 2021

@dbanksdesign there may be a chance I need to use this in one of our projects. Is there a way I can help on this one (other reviews, code refactoring, add tests, etc?) so it gets approved and merged?

dbanksdesign and others added 2 commits January 14, 2021 12:28
Co-authored-by: Cristiano Rastelli <public@didoo.net>
Co-authored-by: Cristiano Rastelli <public@didoo.net>
@dbanksdesign
Copy link
Member Author

@didoo I would love some help with this! I think we need some more tests, more documentation, and updating applicable formats to use this new code. Whichever you'd like to help out on would be greatly appreciated. I also have some time tomorrow I can spend on this and you can take a look.

jbarreiros and others added 15 commits January 22, 2021 15:27
* fix(extend): use given file path for token data
* docs(properties): update properties doc to reflect new additions
* docs(property): adding default metadata to property docs
* docs(properties): adding more context to default metadata
* docs(properties): cleaning up default metadata docs
Adds basePxFontSize platform option which allows you to set the base px font size for pixel transforms. The default remains the same, 16.
…Props keys (#501)

These descriptions are based on the `Default property metadata`
section in the `Properties` docs.
Note: This is a breaking change as it changes the behavior of SD from 2.x
@dbanksdesign
Copy link
Member Author

dbanksdesign commented Jan 26, 2021

Big update:

  • Changed the option name from 'useVariables' to 'keepReferences' so it better matches the methods
  • Moved the option to the options object in a file, to be consistent with other file-level configuration like showFileHeader
  • Updated android/resources format to handle keepReferences
  • Updated scss/variables format to handle keepReferences. In order to do this, because Sass is imperative, we are sorting the allProperties array so all references come last.
  • Addressed @didoo comment about indentation
  • Started to add integration testing to better test the output of formats.
  • Cleaned up some things in our testing

@dbanksdesign
Copy link
Member Author

dbanksdesign commented Jan 29, 2021

Another update:

  • Changed option name to outputReferences
  • Added Swift and Flutter support and integration tests
  • Started some documentation

@dbanksdesign dbanksdesign changed the title feat(format): adding ability to have variables in output feat(format): output references Jan 30, 2021
--color-danger: var(--color-red);
--color-error: var(--color-danger);
}
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this as an example. You might contrast it with what happens if outputReferences is false.

@chazzmoney
Copy link
Collaborator

Nice work. Seriously. LGTM! :shipit:

@dbanksdesign
Copy link
Member Author

Update:

  • Added documentation in the config section about this
  • Platform level configuration can now define an options object with the same options as a file object, and the options cascade
  • Renamed the example

@uptonking
Copy link

uptonking commented Feb 19, 2021

@dbanksdesign
Is there any way to support generate css variables with fallback values?
tokens source:

{ 
  "color": {
    "background": {
      "primary": { 
		"value": "{color.palette.neutral.0.value}" ,
		"fallback":"red"
		}
    }
  }
}

tokens output:

/** simple css var */
--color-background-primary: var(--my-var, red);

/* complex var may not be necessary to be implemented */
--color-background-primary: var(--my-var, var(--my-background, pink));

#544 this issue seems similar.

It seems that I have to write custom format.

@dbanksdesign dbanksdesign deleted the variable-output branch March 12, 2021 22:13
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.

10 participants