Remove templates #152

chazzmoney · 2018-10-03T23:55:07Z

First draft at solving #126

This is intended to be a starting point. I'm sure things are not quite right here and we need ot make some updates before this is ready to roll. Would love feedback @didoo @davixyz @dbanksdesign @elliotdickison and anyone else.

Description of changes:

Removed built-in templates and moved them to built-in formats.
Added lots of warning messages anytime someone use a template in their config. Might need to do this better because it probably will show up multiple times. template: 'whatever' still works, but it pops up a warning.
Added a warning message if someone uses registerTemplate, though it too continues to function correctly.
Updated documentation to focus on formats and provide reasoning for removing templates.
Updated tests.formats.all to have the expected
Fixed light logic issue with fonts.css template (if properties or properties.asset is undefined it produces a blank file now; crashed in this case previously)
Switched eslint to ECMAScript 6 to support multi-line string literals (for my warning text)

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

…ling, I'm not setting up some piece of the template-based formats correctly?

davixyz · 2018-10-04T04:22:08Z

👍 !

didoo

Only one important comment (about a possible missed replacement) and some other general comments. But overall, great job @chazzmoney! 🙌

docs/formats.md

didoo · 2018-10-04T21:05:05Z

docs/formats.md

+  name: 'my/format',
+  formatter: template
+});
+


I think that here, or you show how my/format is used, or is better to remove the styleDictionary.buildAllPlatforms(); line, it may be misleading

yeah, I see what you mean. Do you think I should make up an example? Like sass/map? Or would that confuse people more?

for me, since you are talking about creating a format, you don't need to show also how to use it, it's too much information. keep it simple :)

Let me know if you like the new version.

didoo · 2018-10-04T21:06:56Z

docs/formats.md

 ```scss
 $content-icon-email: '\E001';
 .icon.email:before { content:$content-icon-email; }
 ```

 * * *

-### less/variables
+### less/variables 


not important, just a heads up: I see some diffs are just a trailing space (added) at the end of the line

Yeah, this part of the file is generated by JSDoc. Not sure what causing the difference there.

didoo · 2018-10-04T21:08:10Z

docs/formats.md



 Creates a LESS file with variable definitions based on the style dictionary

-**Example**
+**Color-background-base:**: #f0f0f0;  


not sure about this change, is it correct? I see below the @color have been removed

This is is a weird issue with jsdoc-2-md because our 'less' formats have an '@' in the example code which breaks the markdown file generation. Usually the doc generation script runs when we bump the version, at which point I will go and manually fix this part of the markdown file. If someone can figure out how to fix the issue I would be forever indebted to them.

I'm thinking I should resolve this comment. Or is there a specific action we can take on it?

The action we can take on it for now, is to revert just these changes (you can just copy/paste from this file in master) so the documentation site doesn't break.

didoo · 2018-10-04T21:12:22Z

docs/formats.md

+
+Creates an Objective-C plist file
+
+**Todo**


I can work on this, in the future (I have implemented some PLIST for my iOS team in these days :) )

Sweet! That would be excellent!

Should we make an issue for this?

didoo · 2018-10-04T21:21:21Z

lib/buildFiles.js

  _.each(platform.files, function (file) {
    if (file.template) {
-      buildFile(file.destination, file.template.bind(file), platform, dictionary, file.filter);
+      console.error(warningText);
+      buildFile(file.destination, file.format.bind(file), platform, dictionary, file.filter);


👏👏👏

didoo · 2018-10-04T21:22:37Z

lib/cleanFiles.js

  _.each(platform.files, function (file) {
    if (file.template) {
+      console.error(warningText);
      cleanFile(file.destination, file.template.bind(file), platform, dictionary);


file.format ?

I think this will be called if someone uses files: [{template:'foo'}] in their config. My goal was to ensure that they would see this warning but that the SD would still execute as it did previously.

@dbanksdesign could you double check this?

didoo · 2018-10-04T21:27:30Z

lib/register/format.js

@@ -11,6 +11,9 @@
 * and limitations under the License.
 */

+ var fs = require('fs'),
+     _  = require('lodash'),
+     chalk = require('chalk');


we're not using chalk in the code below, right?

Not using any of those, in fact. Migrated the dependencies from template.js but didn't end up needing them.

didoo · 2018-10-04T21:27:45Z

lib/register/format.js

@@ -35,9 +38,10 @@ function registerFormat(options) {
  if (typeof options.name !== 'string')
    throw new Error('transform name must be a string');
  if (typeof options.formatter !== 'function')
-    throw new Error('format formatter must be a function');
+      throw new Error('format formatter must be a function');


extra space (incorrect indentation)

didoo · 2018-10-04T21:29:01Z

scripts/handlebars/templates/formats.hbs

@@ -1,11 +1,10 @@
 # Formats

-Formats are one of the ways to create files that act as interfaces for your style dictionary. For example, you want to be able to
-use your style dictionary in CSS. You can use the `css/variables` template which will create a CSS file with variables from
+Formats are define the output of your created files for your style dictionary. For example, you want to be able to


"are define"

didoo · 2018-10-04T21:38:26Z

docs/formats.md

@@ -70,7 +80,12 @@ styleDictionary.buildAllPlatforms();

 ## Pre-defined Formats

-[lib/common/formats.js](https://github.com/amzn/style-dictionary/blob/master/lib/common/formats.js)
+These are the formats inclded in Style Dictionary by default, pulled from [lib/common/formats.js](https://github.com/amzn/style-dictionary/blob/master/lib/common/formats.js)


scripts/handlebars/templates/formats.hbs

…bug with handling config that still contains template

lib/buildFiles.js

scripts/handlebars/templates/formats.hbs

…ow it used

chazzmoney · 2018-10-05T17:05:39Z

@didoo Thanks for all your feedback!

@elliotdickison @dbanksdesign Can you pull this, install the cli, and test it with the current SDs that you use? I want to test this change HARD before we push it to develop.

didoo · 2018-10-05T19:53:52Z

@chazzmoney I've tested it on my side too. I've used your branch to compile my Badoo design tokens suite, where I have custom formats and templates for everything. Results: zero changes in the generated files 👊 and a lot of red messages in my console 🤡

Perfect!

dbanksdesign · 2018-10-10T22:17:33Z

I will test this change "hard"

dbanksdesign · 2018-10-10T22:22:09Z

First thought, using this on our demo repo, I get that example code block over and over and it makes the console output a bit messy. Could we just show that once at the end?

Same with the registerTemplate console output. We should just say that message once, and then list all the times it is used. Also that output isn't red like using built-in templates.

chazzmoney · 2018-10-11T00:49:46Z

I definitely agree. I would like to use the method I implemented in the error PR to grab them all and output the explanation once and all the found issues in a list at the end. Hoping that I can use it after it gets pulled into develop branch.

dbanksdesign

Whoops I had a review pending that I never submitted. Here ya go. Maybe lets wait until your error stuff is in develop, then rebase this PR.

lib/common/formats.js

lib/register/template.js

dbanksdesign

Some more comments, there are still a few from earlier that are unresolved. We also need to sync this branch with develop and update the tests.

dbanksdesign · 2018-10-23T22:33:27Z

docs/formats.md



 Creates a LESS file with variable definitions based on the style dictionary

-**Example**
+**Color-background-base:**: #f0f0f0;  


The action we can take on it for now, is to revert just these changes (you can just copy/paste from this file in master) so the documentation site doesn't break.

dbanksdesign · 2018-10-23T22:34:03Z

docs/formats.md

+
+Creates an Objective-C plist file
+
+**Todo**


lib/buildFiles.js

lib/cleanDirs.js

lib/common/formats.js

lib/transform/config.js

scripts/generateDocs.js

scripts/handlebars/templates/templates.hbs

test/buildFiles.js

…nto remove-templates

chazzmoney · 2018-10-26T20:14:21Z

test/formats/all.js

+           state: undefined
+        },
+        path: ['color','red']
+      }
    }


Was this change correct?

chazzmoney · 2018-10-26T20:15:47Z

Ok merged everything I think. My brain has not been fully functioning recently so I may have missed things. Definitely have a problem with the snapshots right now... This may have been from a change I made, which I added a comment about here just now.

Hoping to get this sorted before Monday - feedback greatly appreciated!

chazzmoney · 2018-10-26T20:16:15Z

__tests__/formats/all.test.js

+           state: undefined
+        },
+        path: ['color','red']
+      }
    }


Was this change correct?

Kind of.. we will need to update our snapshots, which is fine. We should probably discuss how to better test these formats because it seems a bit weird test all formats against the same dictionary. For example, the output of the format might not make sense with this input. If you look at the snapshots, some have 'NaN' in them or code that would not actually compile. We might want to think about having a different test and snapshot for each format. Maybe we can save that for later. For now we could just update the snapshots, you can do that in Jest's interactive mode when it comes across a failing snapshot test you can update the snapshot

Updated the snapshots. Maybe we should add an issue for your recommendation?

chazzmoney · 2018-10-30T05:09:32Z

Ok, I think we are at a beta / gold master release for this PR. @didoo and @dbanksdesign can you both test this again and see if the messaging is better? Also let me know if I should make it not crazy red.

chazzmoney · 2018-10-30T05:11:29Z

Looks like one of the merges I might have lost the correct date formatting. Or my snapshots?

dbanksdesign · 2018-10-30T18:58:05Z

Tests pass!

chazzmoney · 2018-10-31T21:24:29Z

@didoo @dbanksdesign Let me know how this works on your real projects. I want to make sure the messaging looks right.

chazzmoney · 2018-10-31T21:37:35Z

Also, @dbanksdesign your review is still requesting changes.

didoo · 2018-11-01T00:05:16Z

@chazzmoney this is what I see when I run the build process:

I think is right, no? I really like how you are not only showing a warning, but also suggesting how to fix it. Very well done!

dbanksdesign

LGTM!

* improved error messages for registerTemplate * updated error message * Introduce option to control the generation of the "Do not edit" header (#132) * stage #1 - formats.js * stage #2 - templates * reset changes to template + simplified changes to formats (now the “options” object is assigned to the “file” element) * fixed wrong parameter passed to fileHeader function * updated documentation * updates after PR comments * removing the confusing static-style-guide stuff (#157) * Fixes #72 * handle no command and invalid commands with friendly console output (#156) * Add json5 support (#165) * Removing unnecessary backticks (#172) * Merge Jest Branch (#169) * Jest testing (#133) * moved all the existing tests to Jest * finalised Jest tests for “utils” removing assert dependency * finalised Jest tests for “register” removing assert dependency + moved tests under correct folder * finalised Jest tests for “transform” removing assert dependency + moved tests under correct folder + removed extra file * updated path for “service” files/folders * removed output folder * updated the paths to ignore in the Jest config in package.json * finalised Jest tests for “clean” removing assert dependency + other small changes * added “__output” to the list of folders ignored by Jest * some tunings + more tests * more tests cleanup * fixed test for exportPlatform * fixed last tests, and now all tests are green! * Added first snapshot tests! Yay! * added mock for dates to avoid failing snapshots tests * updated tests * first attempt to fix the UTC date problem on CI (reference: boblauer/MockDate#9) * second attempt to fix the UTC date problem on CI * removed the TZ=UTC env environment to test if is really needed * updated all the occurrences of new Date in the templates * restored linting before running the tests suite * code style fix * fixed wrong porting of the test for buildAllPlatforms * test(all): Fix for all tests to match the date and remove of mockdate (#148) inspiration jestjs/jest#2234 * test(javascript/es6): Add test for es6 (#149) * test: add registerTemplate (#147) * add tests for transform object (#151) * add tests for transform object * split up complex test in multiple smaller tests * Jest flatten props (#163) * Adding tests for lib/utils/flattenProperties.js (#146) * Adding tests for lib/utils/flattenProperties.js * update to use lodash sortby function * update to use lodash sortby function * Add babel-jest (#173) * feat(json-nested): Add JSON nested transform (#167) Added JSON nested transform, Added test for it and Documentation update re #139 * Fix errors and improve error messaging (#158) * updated error messaging. Fixes for issues with references. * adding in didoo's test from #118 * cleanup of terminology * fixed resolveObject to correctly replace multiple references. modified testing suite to reflect new test. * updates per comments by didoo and dbanksdesign * case sensitive, oops. * case sensitive, oops. * minor updates based on PR feedback * merging with develop to ensure we stay synched * removing cli error handling and moving to module * removing per dannys comments * making constants for group errors per Dannys comments * switch to error grouping mindset and naming * switch to error grouping mindset and naming * per danny's comment * fix flush to execute across all groups if called with no group; remove flush on uncaught exceptions to prevent confusion * simplify, simplify, simplify * changed out error naming to message mindset, cleaned out console.log, fixed issues with simplified GroupMessages * sepearate circular reference tests into separate expects * avoid using string so we dont get it confused with String * Deprecating templates (#152) * Displaying a warning when using templates in the config or registerTemplate * Moving built-in templates to formats * Porting over a stragler test (#190) * 2.5.0

@didoo

* improved error messages for registerTemplate * updated error message * Introduce option to control the generation of the "Do not edit" header (#132) * stage #1 - formats.js * stage #2 - templates * reset changes to template + simplified changes to formats (now the “options” object is assigned to the “file” element) * fixed wrong parameter passed to fileHeader function * updated documentation * updates after PR comments * removing the confusing static-style-guide stuff (#157) * Fixes #72 * handle no command and invalid commands with friendly console output (#156) * Add json5 support (#165) * Removing unnecessary backticks (#172) * Merge Jest Branch (#169) * Jest testing (#133) * moved all the existing tests to Jest * finalised Jest tests for “utils” removing assert dependency * finalised Jest tests for “register” removing assert dependency + moved tests under correct folder * finalised Jest tests for “transform” removing assert dependency + moved tests under correct folder + removed extra file * updated path for “service” files/folders * removed output folder * updated the paths to ignore in the Jest config in package.json * finalised Jest tests for “clean” removing assert dependency + other small changes * added “__output” to the list of folders ignored by Jest * some tunings + more tests * more tests cleanup * fixed test for exportPlatform * fixed last tests, and now all tests are green! * Added first snapshot tests! Yay! * added mock for dates to avoid failing snapshots tests * updated tests * first attempt to fix the UTC date problem on CI (reference: boblauer/MockDate#9) * second attempt to fix the UTC date problem on CI * removed the TZ=UTC env environment to test if is really needed * updated all the occurrences of new Date in the templates * restored linting before running the tests suite * code style fix * fixed wrong porting of the test for buildAllPlatforms * test(all): Fix for all tests to match the date and remove of mockdate (#148) inspiration jestjs/jest#2234 * test(javascript/es6): Add test for es6 (#149) * test: add registerTemplate (#147) * add tests for transform object (#151) * add tests for transform object * split up complex test in multiple smaller tests * Jest flatten props (#163) * Adding tests for lib/utils/flattenProperties.js (#146) * Adding tests for lib/utils/flattenProperties.js * update to use lodash sortby function * update to use lodash sortby function * Add babel-jest (#173) * feat(json-nested): Add JSON nested transform (#167) Added JSON nested transform, Added test for it and Documentation update re #139 * Fix errors and improve error messaging (#158) * updated error messaging. Fixes for issues with references. * adding in didoo's test from #118 * cleanup of terminology * fixed resolveObject to correctly replace multiple references. modified testing suite to reflect new test. * updates per comments by didoo and dbanksdesign * case sensitive, oops. * case sensitive, oops. * minor updates based on PR feedback * merging with develop to ensure we stay synched * removing cli error handling and moving to module * removing per dannys comments * making constants for group errors per Dannys comments * switch to error grouping mindset and naming * switch to error grouping mindset and naming * per danny's comment * fix flush to execute across all groups if called with no group; remove flush on uncaught exceptions to prevent confusion * simplify, simplify, simplify * changed out error naming to message mindset, cleaned out console.log, fixed issues with simplified GroupMessages * sepearate circular reference tests into separate expects * avoid using string so we dont get it confused with String * Deprecating templates (#152) * Displaying a warning when using templates in the config or registerTemplate * Moving built-in templates to formats * Porting over a stragler test (#190) * 2.5.0 * Added 'json/flat' format (#192) * Fix: #195 (#196) * updating contributing to reflect the package manager and testing suite correctly (#197) * Add Sass maps formats (#193) * added ‘sass/map-flat’ and ‘sass/map-deep’ formats + updated tests * fixed inconsistend newlines in templates for sass maps * improved recursive processJsonNode function * updated snapshots tests * removed unused function * Better examples (#164) * changed folder structure * removed table in Readme of Basic example (not clear and probably also some cells were wrong) * small update for the Basic example to make it more clear how aliases are referenced * renamed the “npm” example to “npm module” * updated “npm” example to use the same config and properties as the “basic” example * removed license (no sense here) and updated package.json * updated the s3 example making it more similar to other examples and adding some more assets to be uploaded and linked/embedded in tokens * updated logo in main Readme in example folder * updated the Readme for the S3 example * tried to re-organise the “react” folder in two separate folders the web app doesn’t compile * removed spaces from “example” sub-folder * renamed “example” folder to “examples” * removed numbers from “examples” sub-folder names * removed space in sub-folder names * added advanced example on how to use a watcher to auto-rebuild see: #171 * small update to Readme for “auto-rebuild-watcher” * added advanced example on how to have a multi-platform multi-brand suite * added advanced example on how to use custom templates * fixed “watch” npm script declaration * moved packages under “devDependencies” for “custom templates” package * added a comment in an example of the lodash templating syntax * remove invisible characters from Readme * added “clean” npm script call where missing in examples package.json * added .gitignore file where was missing in examples folder * updated the config file for the “npm module” example * added a comment to explain better how the “formatter” function works * updated the “init” command to expose only the possible/meaningful options + updated documentation for the “examples” page * added comment about collecting more examples * updated the Readme for the “examples” folder * updated “version.js” script as per Danny suggestion * added advanced example on how to use custom transforms * updated basic example to use “format” instead of “template” to avoid the alert in console * added advanced example about referencing/aliasing * updated example to show reference to an “object-like” value * removed the advanced examples for react and react native * added a “create react app” example (with Sass) * better config for S3 example * simplified the example for “S3” * re-introduced android + ios in S3 example * added a “assets-base64-embed” example * finalised the “assets-base64-embed” example * updated Readme for “npm” example + fixed the “prepublishOnly” script option (previous one was deprecated) * removed the “create-react-app-sass” example (I’ll add it later in a separate ticket) * updated the documentation * New cut at documentation PR using current develop branch (#198) * New cut at documentation PR using current develop branch * Apply @didoo's suggestions from code review Co-Authored-By: chazzmoney <[email protected]> * updates based on didoo's thoughts * Updating the architecture documentation page (#200) * updates per didoo and dbanks * typo * generation differences * minor fixes and updates * making sure sd init command documentation is correct, for now * updates for clarity around properties and references * fixing up another alias piece * Addressing some comments in architecture diagram (#204) * Final touches on build diagram and architecture (#206) * Final touches on build diagram and architecture * Updating build diagram * Updating build diagram * Configuration doc update * fixing snapshot whitespace issues, discovered actual failing test on merge... * Fixing merge conflict issues * v2.6.0 release (#210)

Charles Dorner added 2 commits October 3, 2018 14:41

first draft of removing templates in favor of formats. Some tests fai…

a708c76

…ling, I'm not setting up some piece of the template-based formats correctly?

better logic test for fonts template

1872bdc

redid formats documentation page with a fine-tooth comb

bae7eb8

didoo reviewed Oct 4, 2018

View reviewed changes

Charles Dorner added 2 commits October 4, 2018 17:10

fixing didoos issues. dropping number of error messages sent. fixing …

19a1b49

…bug with handling config that still contains template

fixing didoos issues. dropping number of error messages sent. fixing …

34f3059

…bug with handling config that still contains template

didoo reviewed Oct 5, 2018

View reviewed changes

lib/buildFiles.js Outdated Show resolved Hide resolved

scripts/handlebars/templates/formats.hbs Outdated Show resolved Hide resolved

Charles Dorner added 3 commits October 5, 2018 09:38

updated format documentation per didoos comments on not needing to sh…

e9fa7df

…ow it used

updated typos and whitespace problems per didoos comments

bd3adbb

merge with updated develop branch

54bea77

dbanksdesign reviewed Oct 17, 2018

View reviewed changes

lib/common/formats.js Outdated Show resolved Hide resolved

lib/register/template.js Outdated Show resolved Hide resolved

chazzmoney changed the base branch from develop to error-messaging October 20, 2018 05:37

This was referenced Oct 21, 2018

Fix errors and improve error messaging #158

Merged

Rewrite to ES6 #153

Closed

Example Handlebars template #181

Closed

Create an example showing how to use a watcher to auto-rebuild #171

Closed

chazzmoney changed the base branch from error-messaging to develop October 24, 2018 01:15

dbanksdesign requested changes Oct 24, 2018

View reviewed changes

dbanksdesign and others added 4 commits October 26, 2018 09:23

Fixing documentation (#182)

b52434a

merge try 1

94cc558

Merge branch 'remove-templates' of github.com:amzn/style-dictionary i…

e005f2a

…nto remove-templates

merge try 2

0aa676d

chazzmoney commented Oct 26, 2018

View reviewed changes

test/formats/all.js Outdated

state: undefined

},

path: ['color','red']

}

}

Copy link

Collaborator Author

chazzmoney Oct 26, 2018

Choose a reason for hiding this comment

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

Was this change correct?

chazzmoney commented Oct 26, 2018

View reviewed changes

Charles Dorner added 2 commits October 29, 2018 14:52

updated snapshots

ddaf438

group template deprecation warnings

defe485

Fixing snapshots (#183)

839748a

Charles Dorner and others added 3 commits October 30, 2018 17:02

only save messages once. Updated format of messaging and specificity.

eab64c5

Fixing formats.md and console formatting (#184)

16ea34b

slightly updated messages

b95b99c

dbanksdesign approved these changes Nov 1, 2018

View reviewed changes

dbanksdesign merged commit 071c6a5 into develop Nov 1, 2018

dbanksdesign deleted the remove-templates branch November 13, 2018 22:38

Remove templates #152

Remove templates #152

Conversation

chazzmoney commented Oct 3, 2018 • edited Loading

davixyz commented Oct 4, 2018

didoo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chazzmoney commented Oct 5, 2018

didoo commented Oct 5, 2018

dbanksdesign commented Oct 10, 2018

dbanksdesign commented Oct 10, 2018 • edited Loading

chazzmoney commented Oct 11, 2018 • edited Loading

dbanksdesign left a comment

Choose a reason for hiding this comment

dbanksdesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chazzmoney commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chazzmoney commented Oct 30, 2018

chazzmoney commented Oct 30, 2018 • edited Loading

dbanksdesign commented Oct 30, 2018

chazzmoney commented Oct 31, 2018

chazzmoney commented Oct 31, 2018

didoo commented Nov 1, 2018

dbanksdesign left a comment

Choose a reason for hiding this comment

chazzmoney commented Oct 3, 2018 •

edited

Loading

didoo left a comment •

edited

Loading

dbanksdesign commented Oct 10, 2018 •

edited

Loading

chazzmoney commented Oct 11, 2018 •

edited

Loading

chazzmoney commented Oct 30, 2018 •

edited

Loading