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

Better examples #164

Merged
merged 45 commits into from
Nov 30, 2018
Merged

Better examples #164

merged 45 commits into from
Nov 30, 2018

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Oct 9, 2018

Issue #136:

Description of changes:

  • updated logo in the main Readme file
  • changed the folder structure in basic, complete, advanced and test (to be discussed)
  • updated the Basic example and its Readme
  • renamed "npm" to "npm module", updated its properties and config to match the "basic" example
  • updated the s3 example making it more similar to other examples and adding some more assets to be uploaded and linked/embedded in tokens
  • tried to re-organise the “react” folder in two separate folders, then decided to remove them completely
  • updated/added the following "advanced" examples (with proper Readmes):
    -- assets-base64-embed
    -- auto-rebuild-watcher
    -- custom-templates
    -- custom-transforms
    -- multi-brand-multi-platform
    -- npm-module
    -- referencing_aliasing
    -- s3

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

@davixyz
Copy link
Contributor

davixyz commented Oct 14, 2018

Really minor thing but, should we remove the spaces on folder names so when we see them in the web we don't get things like 01%20-%20basic/; I think this will also be weird on the terminal when you are trying to cd into the directories with spaces.

Something like this would be great
https://github.com/zeit/next.js/tree/canary/examples

@didoo
Copy link
Contributor Author

didoo commented Oct 15, 2018

@davixyz thanks for the link! and re. the spaces in the folders filenames, I agree with you, was just an attempt to see if that makes sense. If you have time, please see also the other comments here:
#136 (comment)
and any feedback you may have on the questions/doubts I have raised is much appreciated :)

@chazzmoney
Copy link
Collaborator

Didoo, the most recently added example is very impressive. It will be a super useful starting point for lots of folks I think. @elliotdickison I know you did something similar to this in your actual implementation - can you take a look and see if you have any suggestions?

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

I love the direction of this. I just left a few comments. I think overall what this project is making me realize we should think about and codify a template(s) and structure of what good examples are (what you have here) and what examples we should show, and document it. This would help us when writing the examples as well as others who might want to contribute. We might not necessarily need it for this PR, but just some food for thought.

examples/advanced/auto-rebuild-watcher/package.json Outdated Show resolved Hide resolved
examples/advanced/custom-templates/README.md Show resolved Hide resolved

**Notice**: before starting to dig into all the possible customisations that you can have, try the default settings offered by the library, look at the output files, and see if they can suit your needs. Probably they will do. If they don’t, think how you want the output files generated, and see which one of the API methods you can use for that specific scope.

#### Running the example
Copy link
Member

Choose a reason for hiding this comment

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

For this section, should we have instructions how to pull this code down locally to run it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. Mmm, on one side it would be good to have it at the "examples" root level, on the other if someone doesn't read the main Readme they could miss it. I'll think about a solution.

examples/advanced/custom-templates/README.md Show resolved Hide resolved
"template": "android/colors"
}]
},
"ios": {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe for this example, we remove the Android and iOS stuff and focus on web outputs. We should probably add a javascript platform/output here at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the file, check if it's ok now.

examples/advanced/react/.gitignore Outdated Show resolved Hide resolved
examples/advanced/react/package.json Outdated Show resolved Hide resolved
examples/advanced/custom-templates/README.md Outdated Show resolved Hide resolved
@didoo didoo changed the title Better examples [WIP] Better examples Oct 28, 2018
@dbanksdesign
Copy link
Member

I am really excited to get this out and I want to merge this in this week!

I've been pondering the react and react-native examples for a while now... What are people's thoughts on removing the react and react-native examples? My thinking is that these examples are a bit confusing and don't really fit the mold of the other examples. The react and react-naive examples are codebases of how to consume a style dictionary, but there is nothing really special or different about the style dictionary part itself. It is using standard transforms and formats. What is weird too is, in the real world you would probably want to have your style dictionary package separate from implementation, like react, iOS, Android, etc. and these examples have the react codebase in the same place as the style dictionary.

What if in the npm-module example, we include examples of how to consume the npm module in a react/react-native/other-js-framework codebase? Then we aren't writing a whole functioning react app, but just showing the user how to write a style dictionary as an npm module and consume it in their UI package.

Thoughts?

@didoo
Copy link
Contributor Author

didoo commented Nov 12, 2018

OK, since we all agree, I have removed the examples for React + React Native (we can revert them later, if we want to).

Re. your question, @dbanksdesign, I would not add the example to the npm-module example (maybe, just add an explanation on the Readme would be better).
If I imagine someone who doesn't know how to consume an npm module, I imagine someone that probably would use something like Create React App to bootstrap a project. In that case, I would like to see how to use SD with a CRA app. I don't know if that would mean "react codebase in the same place as the style dictionary" again, but from a certain point of view maybe it would be a sensible compromise to offer something React-related in the examples.

At this point, what remains to complete is the S3 example: as I mentioned in the issue:

I have updated the example, "linking" the assets inside the design tokens, not only copying them in the assets folder. I would like to show also how the assets can be embedded (base64) in the token files but I don't understand how.

Essentially, I don't know if the asset/base64 transform works. I've tried, but it didn't work for me. Maybe I am doing something wrong? do you think there is a meaningful way to show how it works in the S3 example?

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@dbanksdesign
Copy link
Member

I got too excited and didn't see your comment. For react, totally agree, I was actually thinking of an example in the README, not like are real example.

For the s3 example, I just ran it locally and it works for me. Maybe we need an example how to use the base64 sass variables you are creating. You can use them in sass like:

.grid {
  width: 20px;
  height: 20px;
  background-image: url('data:image/svg+xml;base64, #{$grid}');
}

@font-face {
  font-family: 'MyFont';
  src: url('data:font/truetype;base64,#{$ttf}');
  font-weight: normal;
  font-style: normal;
}

I think one issue with the assets.scss file you are creating is there is no 'name' transform, so there are a few variables named $ttf. Should cut down on some of the fonts included as well?

Maybe we should split out base64'ing things into its own example?

@didoo
Copy link
Contributor Author

didoo commented Nov 13, 2018

OK, I did a quick spike to have a "create react app" example with Sass (later we can add one with CSS Modules and one with Styled Components): 6b0a719

What do you think? Makes sense?

@didoo
Copy link
Contributor Author

didoo commented Nov 13, 2018

OK, now the S3 example works (it always worked, I was looking in the wrong place). But now I think it would be better to have an explicit example assets-embed (I would have loved to see it the first time I looked into SD). It will not take me long, I'll do asap. Stay tuned.

@didoo
Copy link
Contributor Author

didoo commented Nov 13, 2018

@dbanksdesign I have added the example for the "base-64 embed" of assets (extracted from the S3 example, so they are two different example on two quite different things). I have one "problem" that maybe you can help me with: I want to show in the same example both the assets embedding and the "normal" tokens generation. This is my config file:

{
  "source": ["properties/**/*.json"],
  "platforms": {
    "scss": {
      "transformGroup": "scss",
      "buildPath": "build/scss/",
      "files": [{
        "destination": "variables.scss",
        "format": "scss/variables"
      }]
    },
    "json/asset": {
      "transformGroup": "web",
      "buildPath": "build/json/",
      "files": [{
        "destination": "variables.json",
        "format": "json/asset"
      }]
    },
    "assets/embed/scss": {
      "transforms": ["attribute/cti", "name/cti/kebab", "asset/base64"],
      "buildPath": "build/scss/",
      "files": [{
        "destination": "assets_icons.scss",
        "format": "scss/variables",
        "filter": {
          "attributes": {
            "category": "asset",
            "type": "icon"
          }
        }
      },{
        "destination": "assets_fonts.scss",
        "format": "scss/variables",
        "filter": {
          "attributes": {
            "category": "asset",
            "type": "font"
          }
        }
      }]
    },
    "assets/embed/json": {
      "transforms": ["attribute/cti", "name/cti/kebab", "asset/base64"],
      "buildPath": "build/json/",
      "files": [{
        "destination": "assets_icons.json",
        "format": "json/flat",
        "filter": {
          "attributes": {
            "category": "asset",
            "type": "icon"
          }
        }
      },{
        "destination": "assets_fonts.json",
        "format": "json/flat",
        "filter": {
          "attributes": {
            "category": "asset",
            "type": "font"
          }
        }
      }]
    }
  }
}

what happens is that in this way we are generating twice the variables for the assets. eg.:

// variables.scss
$asset-icon-github: "assets/icons/github.svg";

// assets_icons.scss
$asset-icon-github: $asset-icon-github: "PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5v...

Is there a way to filter/exclude something in the config? In the documentation, it says you can pass a function to the "filter" but this is a JSON, so it's not possible. Is there a way to do something like (pseudocode): filter: return (attributes.category !== asset)?

@dbanksdesign
Copy link
Member

filter in JSON configs work by giving it an object to filter each token by, in the same way that lodash's filter can take an object instead of a function. Unfortunately this means that this is an inclusive filter, so it will only include things that match. We don't have a way to exclude tokens in a JSON config... Do you think the asset-base64-embed is good enough for now? I think because the advanced examples are meant to show a singular concept, like base64ing assets, this example does that well.

@davixyz
Copy link
Contributor

davixyz commented Nov 20, 2018

👍 I think this is a great addition!

Also 👍 to the multiple filters, I've been doing my custom filters at template level to kinda organize tokens but maybe a multiple filter system could help

@didoo didoo changed the title [WIP] Better examples Better examples Nov 28, 2018
@didoo
Copy link
Contributor Author

didoo commented Nov 28, 2018

@dbanksdesign @chazzmoney I have decided to remove from this PR the "Create React App" example, that I added a couple of weeks ago, because I was stuck with the Readme (I couldn't find time to write it properly) and also because I wanted to try to add also "styled-components" and possibly "CSS Modules" in the same example, so making it more generic around how to consume SD tokens in a React app (created via CRA). So I'll create a separate PR later for this.

So, at this point the PR is done, for me. I'll go through the committed files again, to see if I have missed something or something needs some tweaks, but I think we should merge it in one of the next releases (since is documentation, so no effects on the code itself).

@dbanksdesign
Copy link
Member

Having an example with styled components and css modules would be awesome! I'll go through and double check this tomorrow and I think we are good to go!

@chazzmoney chazzmoney merged commit 5095ac4 into amzn:develop Nov 30, 2018
chazzmoney added a commit that referenced this pull request Dec 1, 2018
* 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 <charles@pgina.org>

* 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)
@didoo didoo deleted the better_examples branch January 19, 2019 18:10
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