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: Add node example #228

Merged
merged 5 commits into from
Jan 29, 2019
Merged

feat: Add node example #228

merged 5 commits into from
Jan 29, 2019

Conversation

dbanksdesign
Copy link
Member

Issue #, if available:

Description of changes: Adding a new example to show the power of using node modules as configuration and property files!

Need help on a more descriptive example name though...

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

examples/advanced/node/README.md Outdated Show resolved Hide resolved
examples/advanced/node/README.md Outdated Show resolved Hide resolved
examples/advanced/node/README.md Outdated Show resolved Hide resolved
* components/index.js Uses node module export/require to merge the property files together without Style Dictionary
* components/button/primary.js Extends a default set of properties
* properties/color/core.js Creates a color ramp programmatically based on base colors
* properties/color/background.js Shows different ways to reference other style dictionary values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* properties/color/background.js Shows different ways to reference other style dictionary values
* `properties/color/background.js` Shows different ways to reference other style dictionary values

});

StyleDictionary.registerFormat({
name: "testing",
Copy link
Contributor

Choose a reason for hiding this comment

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

generally, in the examples, I tend to use more meaningful names (eg. custom/android or custom/template/scss), so the code is easier to read and follow. Here for example you could use myRegisteredFormat

// You can still add custom transforms and formats like you
// normally would and reference them in the config below.
StyleDictionary.registerTransform({
name: 'test',
Copy link
Contributor

Choose a reason for hiding this comment

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

generally, in the examples, I prefer to use more meaningful names (eg. custom/android or custom/template/scss), so the code is easier to read and follow. Here for example you could use myRegisteredTransform

// Same with formats, you can now just write them directly to this config
// object. The name of the format is the key.
format: {
myFormat: (dictionary, platform) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

myFormat vs customTransform => myFormat / myTransform or customFormat / customTransform


// Use a reduce function to take the array of keys in baseColor
// and map them to an object with the same keys.
module.exports = Object.keys(baseColors).reduce((ret, color) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

AMAZING!

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

@dbanksdesign I have added a few (minor) comments in the PR. I think is an amazing example of how powerful SD can be. 🙌 for your work. This leads me to a couple of observations:

  • as you said, the name of the folder in the examples is not clear and meaningful enough :) maybe something like "javascript-modules" (but it may be misleading) or "config-properties-extensions-as-javascript-modules".
  • I saw this in the code:
    // Bypassing Style Dictionary's reference resolution completely.
    // This works (kinda):
    background.disabled = background.alt;
    background.low_priority = background.disabled;
    // The only issue is that because you are bypassing the reference
    // resolution, you no longer get the 'original', un-resolved value
    // You would still have to do it the old way if you want to see
    // the original reference in output files:
    background.test = { value: "{color.background.disabled.value}" }
    and I was amazed and scared at the same time. scared because I don't have even idea that SD can do stuff like this, and what it means, so I imagine me making some small changes in the code and breaking behaviors like these without even knowing. the question I have is: is this kind of functionalities covered by tests? both functional and snapshots? can it be covered? I would try to do it but I don't think I can, I don't have such deep knowledge :(

Overall, I think this PR is fantastic.

@dbanksdesign
Copy link
Member Author

Thanks for your comments! I spoke on the phone with @chazzmoney and he had similar concerns as you. We talked about simplifying those crazier there examples like in the property files. So rather than show a lot of different crazy ways, showing less and telling a more concise story. I'll clean it up and send a revision later this week.

didoo and others added 4 commits January 17, 2019 18:48
Co-Authored-By: dbanksdesign <dbanks.design@gmail.com>
Co-Authored-By: dbanksdesign <dbanks.design@gmail.com>
Co-Authored-By: dbanksdesign <dbanks.design@gmail.com>
@dbanksdesign
Copy link
Member Author

Made some updates, @didoo @chazzmoney thoughts?

//
// properties: properties,
platforms: {
custom: {
// Using the custom transforms we defined above
transforms: ['customTransform', 'test'],
transforms: ['attribute/cti', 'myTransform', 'myRegisteredTransform', 'color/hex'],
Copy link
Contributor

Choose a reason for hiding this comment

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

much clearer now!

buildPath: buildPath,
files: [{
destination: 'variables.txt',
destination: 'variables.yml',
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@didoo
Copy link
Contributor

didoo commented Jan 19, 2019

@dbanksdesign I've left a couple of (positive) comments. For me is a GO, now. 👍

@chazzmoney
Copy link
Collaborator

LGTM, hopefully people will look at this and start asking questions.

@chazzmoney chazzmoney merged commit 25b77cf into develop Jan 29, 2019
@dbanksdesign dbanksdesign deleted the feat/add-node-example branch January 29, 2019 20:36
dbanksdesign added a commit that referenced this pull request Feb 5, 2019
* feat: add registerFilter method (#233)
* doc: remove registerTemplate from examples (#232)
* feat: add node example (#228)
* feat: making include accept an array of globs, like source does (#227)
* fix: the naming for Sass/Scss in documentation, tests, formats, fixes #213 (#222)
* fix: handle showFileHeader option in androids formats templates, fixes #237
 (#243)
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.

None yet

3 participants